-
Notifications
You must be signed in to change notification settings - Fork 47
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
Upgrade to Slang 0.13.1 #545
Conversation
The ranges for variable statement changed because the CST shape changed, which affects the ranges in the test snapshots.
This introduced support for EVM built-ins, so this should substantially increase correct parse coverage.
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@nomicfoundation/slang@0.10.1 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## development #545 +/- ##
==============================================
Coverage ? 52.61%
==============================================
Files ? 228
Lines ? 5318
Branches ? 835
==============================================
Hits ? 2798
Misses ? 2282
Partials ? 238 ☔ View full report in Codecov by Sentry. |
server/src/services/semanticHighlight/highlighters/FunctionDefinitionHighlighter.ts
Show resolved
Hide resolved
server/src/services/semanticHighlight/highlighters/ContractDefinitionHighlighter.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few suggestions. Thanks!
This aims to retain the existing behaviour. We should improve and enhance the behaviour but probably using queries instead, otherwise this will involve a bit of manual node selection and rules that can change fairly often.
I wanted to focus on the upgrading the version with the least amount of changes to facilitate the review.
One change that I introduced is that I introduced CST node name checks in the
SymbolVisitor
to better convey which node we want to match, e.g.name
node in the StateVariableDeclaration (instead of a possible second identifier in the= ...
init part). Some CST node ranges were changed, so I had to modify the snapshot data.Next step should probably include migrating to queries for both perf and completeness' sake (i.e.
FunctionDefinition>FunctionName
that can be eitherIdentifier
or a keyword).It's worth noting that this version correctly parses contextual keywords (like
from
) and supports parsing EVM built-ins, so it should parse correctly a lot more code than the previous 0.10 version.Part of NomicFoundation/slang#639