Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(semantic): transform checker check unresolved references #5096

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Aug 22, 2024

Transform checker check root unresolved references.

The transform checker is now checking pretty much everything it can.

Only fields of ScopeTree and SymbolTable that it's not checking are those which contain AstNodeIds, because transformer does not create node IDs at present:

  • ScopeTree::node_ids
  • SymbolTable::declarations
  • Reference::node_id

Checking also only proceeds in "from AST" direction.

i.e. for each SymbolId which appears in the AST, we check everything about that symbol. But we don't go through all the "rows" in SymbolTable and check if there are any extra symbols in the table which aren't in the AST.

Presumably transformer will leave a lot of old symbols lying around in SymbolTable (ditto scopes and references). We'd need to add ScopeFlags::Deleted, SymbolFlags::Deleted and ReferenceFlags::Deleted for the transformer to be able to "delete" existing symbols.

Copy link

graphite-app bot commented Aug 22, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@overlookmotel overlookmotel force-pushed the 08-22-refactor_semantic_clean_up_transform_checker branch from 7f9bea9 to 37d8972 Compare August 22, 2024 19:47
@overlookmotel overlookmotel force-pushed the 08-22-fix_semantic_transform_checker_check_unresolved_references branch from 6fb2d78 to 60c507d Compare August 22, 2024 19:47
Copy link

codspeed-hq bot commented Aug 22, 2024

CodSpeed Performance Report

Merging #5096 will not alter performance

Comparing 08-22-fix_semantic_transform_checker_check_unresolved_references (9134391) with main (56cc481)

Summary

✅ 29 untouched benchmarks

@overlookmotel overlookmotel force-pushed the 08-22-fix_semantic_transform_checker_check_unresolved_references branch from 60c507d to d7fb634 Compare August 22, 2024 20:19
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Aug 22, 2024
Copy link

graphite-app bot commented Aug 22, 2024

Merge activity

  • Aug 22, 7:39 PM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Aug 22, 7:39 PM EDT: Boshen added this pull request to the Graphite merge queue.
  • Aug 22, 7:52 PM EDT: The Graphite merge queue removed this pull request due to downstack failures on PR #5092.
  • Aug 22, 7:52 PM EDT: The Graphite merge queue removed this pull request due to downstack failures on PR #5092.
  • Aug 22, 8:17 PM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Aug 22, 8:29 PM EDT: overlookmotel added this pull request to the Graphite merge queue.
  • Aug 22, 8:32 PM EDT: The Graphite merge queue removed this pull request due to downstack failures on PR #5093.
  • Aug 22, 8:32 PM EDT: The Graphite merge queue removed this pull request due to downstack failures on PR #5093.
  • Aug 23, 3:47 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Aug 23, 3:52 AM EDT: overlookmotel added this pull request to the Graphite merge queue.
  • Aug 23, 3:57 AM EDT: overlookmotel merged this pull request with the Graphite merge queue.

@Boshen Boshen force-pushed the 08-22-refactor_semantic_clean_up_transform_checker branch from 37d8972 to c58f88a Compare August 22, 2024 23:44
Boshen pushed a commit that referenced this pull request Aug 22, 2024
Transform checker check root unresolved references.

The transform checker is now checking pretty much everything it can.

Only fields of `ScopeTree` and `SymbolTable` that it's *not* checking are those which contain `AstNodeId`s, because transformer does not create node IDs at present:

* `ScopeTree::node_ids`
* `SymbolTable::declarations`
* `Reference::node_id`

Checking also only proceeds in "from AST" direction.

i.e. for each `SymbolId` which appears in the AST, we check everything about that symbol. But we *don't* go through all the "rows" in `SymbolTable` and check if there are any extra symbols in the table which aren't in the AST.

Presumably transformer will leave a lot of old symbols lying around in `SymbolTable` (ditto scopes and references). We'd need to add `ScopeFlags::Deleted`, `SymbolFlags::Deleted` and `ReferenceFlags::Deleted` for the transformer to be able to "delete" existing symbols.
@Boshen Boshen force-pushed the 08-22-fix_semantic_transform_checker_check_unresolved_references branch from d7fb634 to 165767b Compare August 22, 2024 23:45
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Aug 22, 2024
@overlookmotel overlookmotel force-pushed the 08-22-refactor_semantic_clean_up_transform_checker branch from c58f88a to e018069 Compare August 23, 2024 00:16
overlookmotel added a commit that referenced this pull request Aug 23, 2024
Transform checker check root unresolved references.

The transform checker is now checking pretty much everything it can.

Only fields of `ScopeTree` and `SymbolTable` that it's *not* checking are those which contain `AstNodeId`s, because transformer does not create node IDs at present:

* `ScopeTree::node_ids`
* `SymbolTable::declarations`
* `Reference::node_id`

Checking also only proceeds in "from AST" direction.

i.e. for each `SymbolId` which appears in the AST, we check everything about that symbol. But we *don't* go through all the "rows" in `SymbolTable` and check if there are any extra symbols in the table which aren't in the AST.

Presumably transformer will leave a lot of old symbols lying around in `SymbolTable` (ditto scopes and references). We'd need to add `ScopeFlags::Deleted`, `SymbolFlags::Deleted` and `ReferenceFlags::Deleted` for the transformer to be able to "delete" existing symbols.
@overlookmotel overlookmotel force-pushed the 08-22-fix_semantic_transform_checker_check_unresolved_references branch from 165767b to 0993b30 Compare August 23, 2024 00:17
@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Aug 23, 2024
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Aug 23, 2024
@overlookmotel overlookmotel force-pushed the 08-22-refactor_semantic_clean_up_transform_checker branch from e018069 to 13c961c Compare August 23, 2024 07:42
overlookmotel added a commit that referenced this pull request Aug 23, 2024
Transform checker check root unresolved references.

The transform checker is now checking pretty much everything it can.

Only fields of `ScopeTree` and `SymbolTable` that it's *not* checking are those which contain `AstNodeId`s, because transformer does not create node IDs at present:

* `ScopeTree::node_ids`
* `SymbolTable::declarations`
* `Reference::node_id`

Checking also only proceeds in "from AST" direction.

i.e. for each `SymbolId` which appears in the AST, we check everything about that symbol. But we *don't* go through all the "rows" in `SymbolTable` and check if there are any extra symbols in the table which aren't in the AST.

Presumably transformer will leave a lot of old symbols lying around in `SymbolTable` (ditto scopes and references). We'd need to add `ScopeFlags::Deleted`, `SymbolFlags::Deleted` and `ReferenceFlags::Deleted` for the transformer to be able to "delete" existing symbols.
@overlookmotel overlookmotel force-pushed the 08-22-fix_semantic_transform_checker_check_unresolved_references branch from 0993b30 to 7a99e0d Compare August 23, 2024 07:46
@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Aug 23, 2024
Base automatically changed from 08-22-refactor_semantic_clean_up_transform_checker to main August 23, 2024 07:49
Transform checker check root unresolved references.

The transform checker is now checking pretty much everything it can.

Only fields of `ScopeTree` and `SymbolTable` that it's *not* checking are those which contain `AstNodeId`s, because transformer does not create node IDs at present:

* `ScopeTree::node_ids`
* `SymbolTable::declarations`
* `Reference::node_id`

Checking also only proceeds in "from AST" direction.

i.e. for each `SymbolId` which appears in the AST, we check everything about that symbol. But we *don't* go through all the "rows" in `SymbolTable` and check if there are any extra symbols in the table which aren't in the AST.

Presumably transformer will leave a lot of old symbols lying around in `SymbolTable` (ditto scopes and references). We'd need to add `ScopeFlags::Deleted`, `SymbolFlags::Deleted` and `ReferenceFlags::Deleted` for the transformer to be able to "delete" existing symbols.
@overlookmotel overlookmotel force-pushed the 08-22-fix_semantic_transform_checker_check_unresolved_references branch from 7a99e0d to 9134391 Compare August 23, 2024 07:52
@graphite-app graphite-app bot merged commit 9134391 into main Aug 23, 2024
25 checks passed
@graphite-app graphite-app bot deleted the 08-22-fix_semantic_transform_checker_check_unresolved_references branch August 23, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants