Remove non-struct NatSpec from namespaced storage layout compilation

[ericglau]

When making a modified copy of solc input to extract namespaced storage layout, many previously encountered bugs were due to unexpected compilation errors when NatSpec no longer matches the modified source for various reasons (e.g. parameter or return parameter changes in functions, or NatSpec being orphaned after nodes are deleted).

We were previously unable to reliably delete these NatSpec nodes using the Solidity AST, because some NatSpec nodes are not included in the Solidity AST, for example NatSpec-style comments which were orphaned in the first place.

Now with Slang, if Slang supports the current platform where this plugin is run from, we can use it to do a separate parse of the source code and another set of modifications: we traverse the CST to look for any NatSpec not associated with a struct, and delete them. This would increase reliability on platforms where Slang is supported.

However, it is important to only do this if the NatSpec is indeed not part of a struct -- we must keep all struct NatSpec since those may contain ERC7201 storage location annotations, and this modified copy of solc input is used for namespace validations in the Hardhat Upgrades plugin.

[frangio]

I've only had a cursory look at this PR, it looks reasonable but someone else might want to review it more thoroughly.

[frangio]

I would say though that this seems very wrong to me. Slang should self-report on what platforms it supports to avoid issues when platforms are added or removed.

[ericglau]

Will suggest this for Slang.

[alcuadrado]

Importing Slang in an unsupported platform will actually throw an error, so to support other platforms you should only import it dynamically, which may be somewhat annoying.

You can still use import type in the top-level scope of your files though.

At the same time, this could also help you build an alternative isSlangSupported function, like this:

function isSlangSupported() : boolean {
   try {
     require("@nomicfoundation/slang");
   } catch (e) { 
      if ("message" in e && typeof e.message == "string") {
         return e.message === 'Failed to load native binding' || e.message.startsWith('Unsupported architecture') || e.message.startsWith('Unsupported OS');
      }
      
      throw e;
   }
}

This was written inside the GitHub UI, so it may not even compile, but it shows the general idea. The error messages are available in @nomicfoundation/slang/napi-bindings/generated/index.js. Unfortunately, the generated bindings use a plain Error.

Finally, two other notes:

• We've seen virtually no usage of Hardhat in platforms other than the ones listed here, so we strongly believe that it covers most, if not all, the Ethereum ecosystem.
• Once we switch to wasm, this won't be needed anymore :)

[OmarTawfik]

Thanks for the suggestion, and I agree with you that this should be improved. We are in the process of moving to WASM (in Q3), which will remove the need for platform-specific binaries in any case, so that won't be needed in the future.

In the meantime, the list of supported platforms can be read from the package.json dependencies, and listed here as well:

https://www.npmjs.com/package/@nomicfoundation/slang?activeTab=dependencies

[ericglau]

Thanks for the feedback, everyone. I've changed this to try importing Slang dynamically and if it has an error, we can simply assume the platform is not supported and skip this code path (this way we can avoid checking the error messages since those are not part of the stable API).

[ericglau]

One topic to consider is whether this removal of all NatSpec aside from those attached to struct definitions would cause the plugin to lose information about other annotations. For example, the @custom:oz-* annotations for disabling checks or renaming types.

My assessment is that these are unrelated. The solc output resulting from this modified source code is only used to extract the storage layout here. This is an optional parameter to the validate function, and most of the checks that access the NatSpec annotations occur in the section above.

When extracting storage layouts, there are additional possible annotations to allow renaming/retyping, which are checked here and here. These come from the original storageLayout and contractDef parameters in this extractStorageLayout function, and do not come from namespaced compilation in namespacedContext. (Additionally, Solidity does not currently support struct member NatSpec annotations so these annotations currently do not exist in namespaces.)

The only place that uses annotations from the namespaced output should be in loadNamespaces, where the context can come from the namespaced compilation and this is used to find ERC-7201 annotations.

In summary, deleting NatSpec other than for struct definitions is fine for namespaced storage layout purposes.

[ernestognw]

LGTM

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@nomicfoundation/[email protected]	filesystem, shell	+9	36.2 MB	nomic-foundation-publisher

View full report↗︎