Conversation
|
I'm not fully convinced about this one. Let me first check if I'm understanding correctly: We are now using Slang's In addition to that, we can filter that list returned by Slang so that it only includes "milestone" versions; that is, versions where some syntax actually changed. This is just an optimization, as far as I understand. There are two things about this approach I don't quite like:
So, unless you feel strongly that this is a good trade-off, I'd propose just using the latest version inferred by Slang. Regardless of this discussion, we should improve our parsing errors to include the parser version we used (I don't think we are doing it now), and that should make that edge case easier to fix when/if it happens. |
|
the only other example I was having also in mind is code without pragma and choosing the version based on the syntax only. keeping up this list will be indeed an extra load to keep in mind every time there is an update of the only issue for me is that people will rely mostly on this feature and in all of this cases it will fail. |
I assume that a user writing a pragma But because Solc releases don't necessarily follow SemVer (breaking changes in patch releases), we might want to use the first/earliest inferred version by Slang, not the last/latest. This is because we have the ability to precisely "use the syntax/semantics/APIs of Prettier is not using Solc here, so it will never benefit from "possible future bug fixes/optimizations" in any case.
If you still need this, it can also be generated automatically. A
This is a great point. Something that says:
If there will be cases where the correct version can never be inferred, adding an override in config might help there! |
c87faf6 to
7b8530c
Compare
I don't think this happens a lot in practice (it's a bad idea, and also you get an annoying compilation warning). So if we are just using the latest supported version (vs. inferring it), I think it's fine.
This is an interesting idea. The problem is that tools (or at least Hardhat, but I think Truffle used to do something similar) infer the latest possible version. For example, in Hardhat if you have tl;dr, I think using the latest supported version is simple and consistent with other tools. Also, I wouldn't consider this behavior something that needs to be super stable. Ideally we don't change it later, but if this turns out to be a bad decision, it would be fine to change it because you can always specify the compiler you want to use yourself to override the default behavior. |
|
Ok sorry for the few days AFK, To explain a bit my approach, I'm thinking of supporting developers while minimising the configuration that needs to be used with I consider the pragmaless scenario because I still want the user to see the formatting on an unfinished piece of code or for example when writing a MD file with an embedded solidity example. I also consider the use case where the developer started at a pragma (ie. I recon my current solution using a list of the breaking changes in the syntax has a few possible issues:
All of these could be solved by @OmarTawfik's offering of supporting this list (unofficially) based on their JSON file which is autogenerated by their own development of their parser's Languages. I also want to say that we did not have this issue with our old parser because it didn't rely on version, so we never got to discuss which is our end user. Happy to keep this conversation open so we can adapt the decisions in the future. |
7b8530c to
5301dd6
Compare
|
I just added a new commit removing the iteration through the list. I had to also provide specific versions for a few of the tests that were relying on version inference. I also thought about another reason to have the list (hopefully automated from the |
d871b1d to
61c2f23
Compare
61c2f23 to
ff18c5d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR replaces our custom language inference tool with Slang's built‐in capabilities while retaining fallback logic for very specific syntax cases. It also updates test cases and snapshots to include explicit compiler versions and adjusts parser instantiation and error messaging accordingly.
- Updated runFormatTest calls to supply a compiler version.
- Adjusted parser creation logic and error handling in src/slangSolidityParser.ts and src/slang-utils/create-parser.ts.
- Removed outdated pragma statements from snapshots and Solidity source files.
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/format/MemberAccess/*.js | Updated test to include compiler version for Slang. |
| tests/format/IndexOf/.js, FunctionDefinitions/*.js, etc. | Similar updates to pass compiler version and update snapshots accordingly. |
| tests/format/Comments/.snap, AllSolidityFeatures/*.snap | Removed outdated pragmas and updated snapshot references. |
| tests/config/run-format-test.js | Changed property access in parser creation to account for new object shape. |
| src/slangSolidityParser.ts | Updated parser destructuring; removed explicit error throwing for invalid parse. |
| src/slang-utils/create-parser.ts | Refactored parser creation, changed semver filter from maxSatisfying to minSatisfying, and updated error messages. |
| package.json | Upgraded dependency versions for ESLint and related tooling. |
Comments suppressed due to low confidence (1)
src/slangSolidityParser.ts:24
- The removal of the error throw when parseOutput is invalid may lead to silent failures if parsing fails. Consider reintroducing error handling to ensure that invalid parse outputs are not silently ignored.
}
|
|
||
| if (!result.parseOutput.isValid()) | ||
| throw new Error( | ||
| 'We encoutered the following syntax error:\n\n\t' + |
There was a problem hiding this comment.
The word 'encoutered' appears to be misspelled; consider correcting it to 'encountered'.
| 'We encoutered the following syntax error:\n\n\t' + | |
| 'We encountered the following syntax error:\n\n\t' + |
There was a problem hiding this comment.
I enabled the copilot review because I saw this typo and was curious if it would catch it and if it would catch something else.
fvictorio
left a comment
There was a problem hiding this comment.
Tiny comment. Feel free to merge with or without applying that suggestion.
| } | ||
| const result = parserAndOutput( | ||
| text, | ||
| inferredRanges[inferredLength === supportedLength ? inferredLength - 1 : 0] |
There was a problem hiding this comment.
Can inferredRanges be empty? I think this can only happen if you have two pragma statements whose intersection is empty, which is very unlikely, but maybe throwing an assertion error wouldn't be a bad idea.
There was a problem hiding this comment.
so far slang just throws an error in this case, but I could add an check here in case slang changes this behaviour
Slang also offers a new tool to infer the language using pragma statements.
With this we can drop out our custom Language inference tool but we have to keep some of it's logic for the cases where there is a very broad set of supported languages but the code has a very specific syntax that was not taken in consideration when defining the pragma statement. The slang team stated that this is beyond the scope of their tool.
Another change is that the Slang documentation contains a list of every version that has a syntax change. With this information we can change our old behaviour of using minor versions as syntax change steps.
Also in the previous version we were caching the last successful
parser. Since prettier's function are async, multiple calls can be made with different pragmas and syntaxes and this could create issues if theparsercan be changed in a non synchronous way.