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

Slang 0.18.3 #1056

Merged
merged 36 commits into from
Nov 2, 2024
Merged

Slang 0.18.3 #1056

merged 36 commits into from
Nov 2, 2024

Conversation

Janther
Copy link
Contributor

@Janther Janther commented Oct 23, 2024

adding support for slang v0.18.0 and WebAssembly.

Main changes:

  • adapting to small changes in the slang API
  • dropping (temporarily hopefully) support for UMD
  • bundling an esm package
  • removing all the use of the slang AST that is not necessary

Janther added 23 commits October 9, 2024 20:02
  - update module resolution to NodeNext needed by slang
  - update all imports to slang to the latest one
  - update all import type to include the extension
  - TerminalNode#text is no longer available so we are using TerminalNode.unparse() instead.
  - renamed ABICoderPragma to AbicoderPragma
  - using provided `asNonterminalNode` method instead of manually casting the tree.
  - using Parser instead of Language
  - some accessors became methods and some methods became accessors
  - using isNodeTerminal() since Node.type was removed
  - cst.children used to return Node[], now it returns Edge[]
@Janther Janther changed the title Slang 0.18.2 Slang 0.18.3 Oct 28, 2024
@Janther Janther requested a review from fvictorio October 29, 2024 04:58
@@ -23,18 +23,8 @@ const warnDeprecation = once(() => {
return true;
});
Copy link

@OmarTawfik OmarTawfik Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this PR is explicitly dropping support for v2, I wonder if this should be kept, specifying 3.0.0 instead?

Same question for src/slangPrinter.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prettier 2.2.X would still load the plugin and fail miserably when using it, so we needed the check for >=2.3.0 to show the reasons why it was failing.
Now prettier 2.X.X won't even load the plugin so the code will never reach this code.
there is a check in peerDependencies in package.json and I'll add this info on the readme.

const prettier = await getPrettier();

const prettierPath = path.dirname(require.resolve("prettier"));
const pluginPrefix = satisfies(prettier.version, "^2.3.0")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remaining v2 support. I wonder if it should be removed?

Same question for src/common/backward-compatibility.js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. thanks for finding this.

});
}

test('should throw an error if there are incompatible ranges', function () {
test.skip('should throw an error if there are incompatible ranges', function () {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is skipped. not sure if that was intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intended to be skipped for now.

before we were joining pragma versions into a single string and it was straight forward to find an inconsistency, but as you pointed out, there are some edge cases where this approach leads to an error. Now we evaluate each pragma version against slang's supported versions, and now an incompatibility in the pragma versions is no different from an eager developer chasing to use the latest solidity release which might not be supported by slang just yet but with a syntax fully supported by slang.

so we might choose to remove this test as it might not be a responsibility of prettier-solidity or we will have to add extra steps in the version inference to double check for the validity of the pragmas.

}

function tryToCollectPragmas(text: string, version: string): string[] {
const language = Parser.create(version);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion to remove the need for the extra dependency, and to be able to handle things like newlines/other corner cases:

const fileCursor = parser.parse(NonterminalKind.SourceUnit, source).createTreeCursor();
const values = [];

while (fileCursor.goToNextNonterminalWithKind(NonterminalKind.VersionExpressionSets)) {
  const versionCursor = fileCursor.spawn();
  let value = "";

  while (versionCursor.goToNextTerminal()) {
    const terminal = versionCursor.node.asTerminalNode()!;
    if (TerminalKindExtensions.isTrivia(terminal.kind)) {
      value += " "; // to break up any possible consecutive identifiers
    } else {
      value += terminal.unparse();
    }
  }

  values.push(value);
}

Copy link
Contributor Author

@Janther Janther Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using this and tests aren't passing.
I suspect this is happening when there are no pragmas and the syntax is old.

Copy link

@OmarTawfik OmarTawfik Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That syntax node should be consistent across all Solidity versions:
https://nomicfoundation.github.io/slang/0.18.3/solidity-specification/01-file-structure/03-pragma-directives/#VersionExpressionSets

I wonder if you have a commit # I can try out? Happy to help if I can debug the failure..

@@ -1,18 +1,27 @@
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we have integration tests that run this inside a browser?
I see that the README.md says _Disabled during v2.0.0-beta_. Is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were running all of our tests in a virtual empty javascript context. But had to drop it because it wouldn't load wasm files and that configuration was design for commonjs.
I'll research adding an integration test with a browser. karma seems to be deprecated now.

I'll update the Readme

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used puppeteer before for CI testing, and IIRC, you can load an .html in-memory and just query the elements there.

Copy link

@OmarTawfik OmarTawfik left a 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/question. Thank you! 🚀🚀

@Janther Janther force-pushed the slang-0.18.0 branch 2 times, most recently from 87a3d86 to a1a6198 Compare October 30, 2024 01:42
@Janther Janther merged commit 1cf3dfd into v2 Nov 2, 2024
7 checks passed
@Janther Janther deleted the slang-0.18.0 branch November 2, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants