fix(semantic): remove check_unresolved_references

[Dunqing]

As I said in #5298 (comment). The unresolved_references check is incorrect. In the meantime, I found that check_reference contained the unresolved_references check, so I removed it

[Dunqing]

• fix(semantic): remove check_unresolved_references #5301 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Dunqing and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #5301 will not alter performance

_{Comparing 08-28-fix_semantic_remove_check_unresolved_references (32dc517) with main (7fa2fa3)}

Summary

✅ 28 untouched benchmarks

[overlookmotel]

I'm afraid I disagree with you on this one.

First off, the test failure discussed in #5298 (review) is correct, as far as I can see. We have failed to delete an unresolved reference when we should have, and the checker flags that.

Secondly, check_unresolved_references is covering a different set of cases from check_references. check_references starts from a list of all the IdentifierReferences which are in the AST and checks they have the correct SymbolId. check_unresolved_references comes at it from the opposite direction - it loops through ScopeTree::root_unresolved_references and checks (a) there aren't any keys in that there shouldn't be and (b) they each have the right ReferenceIds recorded against them.

In the case of #5298 (review), the problem is that we've deleted the IdentifierReference from AST, but not deleted the corresponding reference from root_unresolved_references. So check_references could not catch this, as it starts from the AST, and there is nothing in the AST to start from (as it was deleted).

[Dunqing]

Yes, I know we should remove them. but since Semantic doesn't provide remove action, this checking is always a mismatch, right? Also, symbol and scope have the same problem. But we haven't checked them what unremoved.

[overlookmotel]

Sorry I was rushing out of the house yesterday, so just whacked out #5305, but didn't have time to reply here.

since Semantic doesn't provide remove action

It does now! #5305

Also, symbol and scope have the same problem. But we haven't checked them what unremoved.

That is true. I left that out of the transform checker for now as currently we have no way to flag scopes, symbols and references as deleted (#5097). Adding that'll be a bit of work, so I thought best just punt the problem for now - and it only matters if you iterate over ScopeTree / SymbolTable directly, which mostly we don't.

Whereas, for unresolved references, even though we didn't have an API for deleting them, I knew it wouldn't be hard to add one.

So, in summary... in my view we should delete unresolved references, #5305 gives us an API to do that, and the transformer checker should check it's done correctly. So I think we should close this PR.

[Dunqing]

So, in summary... in my view we should delete unresolved references, #5305 gives us an API to do that, and the transformer checker should check it's done correctly. So I think we should close this PR.

I agree