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.

[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.

[socket-security[bot]]

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↗︎