Use the ast edge label when building the ast node label #688
Conversation
aeisenberg
force-pushed
the
aeisenberg/ast-label
branch
from
6bf3128 to
ef0c8ea
Compare
extensions/ql-vscode/CHANGELOG.md
Outdated
| @@ -10,6 +10,7 @@ | |||
| - Whenever the extension restarts, orphaned databases will be cleaned up. These are databases whose files are located inside of the extension's storage area, but are not imported into the workspace. | |||
| - After renaming a database, the database list is re-sorted. [#685](https://github.com/github/vscode-codeql/pull/685) | |||
| - Add a `codeQl.resultsDisplay.pageSize` setting to configure the number of results displayed in a single results view page. Increase the default page size from 100 to 200. [#686](https://github.com/github/vscode-codeql/pull/686) | |||
| - Update the AST Viewer so that edge names are included in each node label. So far, only C/C++ databases take advantage of this change. [#688](https://github.com/github/vscode-codeql/pull/688) | |||
There was a problem hiding this comment.
| - Update the AST Viewer so that edge names are included in each node label. So far, only C/C++ databases take advantage of this change. [#688](https://github.com/github/vscode-codeql/pull/688) | |
| - Update the AST Viewer to include edge labels (if available) in addition to the target node labels. So far, only C/C++ databases take advantage of this change. [#688](https://github.com/github/vscode-codeql/pull/688) |
| @@ -45,17 +45,18 @@ export default class AstBuilder { | |||
| const parentToChildren = new Map<BqrsId, BqrsId[]>(); | |||
| const childToParent = new Map<BqrsId, BqrsId>(); | |||
| const astOrder = new Map<BqrsId, number>(); | |||
| const tupleTargetLabels = new Map<BqrsId, string>(); | |||
There was a problem hiding this comment.
Nit: Something like edgeLabels would make the purpose clearer. Please also add a comment explaining that this is a map from the target node ID to the edge label.
| @@ -65,6 +66,11 @@ export default class AstBuilder { | |||
| parentToChildren.set(sourceId, children = []); | |||
| } | |||
| children.push(targetId); | |||
|
|
|||
| // ignore values that indicate a numeric order. | |||
| if (!Number.isFinite(Number(value))) { | |||
There was a problem hiding this comment.
If we're in the semmle.label case rather than semmle.order, isn't it reasonable to accept a number as the edge label?
There was a problem hiding this comment.
When you get to line 71, you are always looking at semmle.label since this is a switch statement.
If we accept a number as an edge label here, then we get confusing labels in the nodes of the ast viewer tree.
There was a problem hiding this comment.
Because the AST viewer already numbers children?
| @@ -84,9 +90,10 @@ export default class AstBuilder { | |||
| break; | |||
|
|
|||
| case 'semmle.label': { | |||
| const label = [tupleTargetLabels.get(id), value ?? entity.label].filter(e => e).join(': '); | |||
There was a problem hiding this comment.
Clever, but took me a minute. Comment to explain what it's doing, in particular that the UI will show the edge label (if available) and then the target node label? Or pull out edgeLabel and nodeLabel variables to make it clear what the individual values are.
There was a problem hiding this comment.
I can clarify that a bit.
extensions/ql-vscode/src/vscode-tests/no-workspace/contextual/astBuilder.test.ts
Show resolved
Hide resolved
The C PrintAST library now includes the edge name in the AST Viewer tree.
aeisenberg
force-pushed
the
aeisenberg/ast-label
branch
from
ef0c8ea to
2162156
Compare
|
Displaying numbers as the edge labels is redundant with the order and doesn't add any extra information. When I tried it out, the labels were cluttered. |
The C PrintAST library now includes the edge name in the AST Viewer
tree.
Resolves #687
Checklist
@github/docs-content-dsphas been cc'd in all issues for UI or other user-facing changes made by this pull request.