Fix obscure symbol reassociation bug - #457
Conversation
cirras
changed the title
d0b5db0 to
5252de0
Compare
|
Fixed the failing tests and added a new pile of unit tests covering the Tests were failing because we were actually relying on this awful "multiple nodes return the same token index behavior" in several ways. Most notably, the registered location for every name occurrence is an |
fourls
left a comment
fourls
left a comment
There was a problem hiding this comment.
I think this looks good!
As far as I can see we don't rely on this shared-index behaviour anywhere else.
The new SymbolAssociationVisitor tests are really good to have.
We were registering declarations, occurences, and scopes against token indices as if they were unique at the node level. They're not. This led to some tricky situations where scopes in particular would be incorrectly attributed to the wrong nodes. This bug was masked very well because most name resolution occurs during symbol table construction. The problem came about after symbol reassociation. In the analysis phase, on-demand name resolution occurs during expression type resolution. Any analysis rule calling `ExpressionNode::getType` was potentially impacted.
5252de0 to
7bd6758
Compare
|
Realized I did the classic "wrong year in license header" mistake with the new file. |
fourls
left a comment
fourls
left a comment
There was a problem hiding this comment.
I didn't notice, good catch 😆
2 participants
This PR fixes an obscure bug related to the 2-pass semantic analysis we're doing where we build the symbol table in one pass, throw away ASTs as we go, and re-parse source files (reassociating symbol information) in a second pass.
For symbol reassociation purposes, we were registering declarations, occurences, and scopes against token indices as if they were unique at the node level. They're not.
This led to some tricky situations where scopes in particular would be incorrectly attributed to the wrong nodes. This bug was masked very well because most name resolution occurs during symbol table construction.
The problem came about after symbol reassociation. In the analysis phase, on-demand name resolution occurs during expression type resolution. Any analysis rule calling
ExpressionNode::getTypewas potentially impacted.