feat(diagnostics): diagnose duplicate virtual specifiers by teofr · Pull Request #1832 · NomicFoundation/slang

teofr · 2026-06-05T16:57:42Z

Rejects definitions carrying more than one virtual keyword, matching solc's behavior. Emits a new syntax/multiple-virtual-specifiers diagnostic ("Only a single virtual specifier can be provided.") on every virtual keyword after the first.

Implements solc error codes 6879 and 2662

Reject functions, fallback/receive functions, and modifiers that carry more than one `virtual` keyword, matching solc's ParserError 6879/2662. Closes the `virtual` part of the SDR[1730] validation TODO in the IR builder.

changeset-bot · 2026-06-05T16:57:47Z

⚠️ No Changeset found

Latest commit: 1f53a14

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copilot

Pull request overview

Adds a new v2 syntax diagnostic to match solc behavior when virtual is specified more than once on the same definition, emitting syntax/multiple-virtual-specifiers for each virtual after the first.

Changes:

Introduces MultipleVirtualSpecifiers diagnostic kind (syntax/multiple-virtual-specifiers) with an error severity and message aligned to the PR description.
Updates the v2 CST→IR builder to extract the first virtual keyword and emit a diagnostic on each subsequent virtual for functions, fallback/receive functions, and modifiers.
Adds new diagnostics snapshot fixtures and test wiring covering functions, fallback, receive, and modifiers.

Reviewed changes

Copilot reviewed 8 out of 17 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/receive_functions/input.sol	New snapshot input covering duplicate `virtual` on `receive()`.
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/receive_functions/generated/solc/0.8.0-failure.txt	Expected `solc` failure output for duplicate `virtual` on `receive()`.
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/receive_functions/generated/slang/0.8.0-failure.txt	Expected Slang diagnostic output for duplicate `virtual` on `receive()`.
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/modifiers/input.sol	New snapshot input covering duplicate `virtual` on a modifier.
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/modifiers/generated/solc/0.8.0-failure.txt	Expected `solc` failure output for duplicate `virtual` on a modifier.
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/modifiers/generated/slang/0.8.0-failure.txt	Expected Slang diagnostic output for duplicate `virtual` on a modifier.
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/functions/input.sol	New snapshot inputs covering duplicate `virtual` on regular functions (including multiple duplicates).
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/functions/generated/solc/0.8.0-failure.txt	Expected `solc` failure outputs for duplicate `virtual` on functions (multiple diagnostics).
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/functions/generated/slang/0.8.0-failure.txt	Expected Slang diagnostic outputs for duplicate `virtual` on functions (multiple diagnostics).
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/fallback_functions/input.sol	New snapshot input covering duplicate `virtual` on `fallback()`.
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/fallback_functions/generated/solc/0.8.0-failure.txt	Expected `solc` failure output for duplicate `virtual` on `fallback()`.
crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/multiple_virtual_specifiers/fallback_functions/generated/slang/0.8.0-failure.txt	Expected Slang diagnostic output for duplicate `virtual` on `fallback()`.
crates/solidity-v2/outputs/cargo/tests/src/diagnostics_output/snapshots.generated.rs	Registers the new snapshot test suite under `syntax/multiple_virtual_specifiers`.
crates/solidity-v2/outputs/cargo/ir/src/ir/builder/mod.rs	Implements duplicate-`virtual` detection via a shared `extract_first_virtual` helper and emits the new diagnostic.
crates/solidity-v2/outputs/cargo/common/src/diagnostics/kinds/syntax/multiple_virtual_specifiers.rs	Adds the new `MultipleVirtualSpecifiers` diagnostic kind (code + message + severity).
crates/solidity-v2/outputs/cargo/common/src/diagnostics/kinds/syntax/mod.rs	Wires the new diagnostic kind into the syntax diagnostics module and enum.
crates/solidity-v2/outputs/cargo/common/generated/public_api.txt	Updates the public API surface to include the new diagnostic type/enum variant.

teofr · 2026-06-05T17:00:12Z

        let mutability = self
            .extract_mutability_specifier(&source.attributes.elements)
            .unwrap_or(output::FunctionMutability::NonPayable);
-        let virtual_keyword = source.attributes.elements.iter().find_map(|attribute| {
-            if let input::FunctionAttribute::VirtualKeyword(virtual_keyword) = attribute {
-                Some(self.build_virtual_keyword(virtual_keyword))
-            } else {
-                None
-            }
-        });
+
+        let virtual_keyword =
+            self.extract_first_virtual(source.attributes.elements.iter().filter_map(|attribute| {
+                if let input::FunctionAttribute::VirtualKeyword(virtual_keyword) = attribute {
+                    Some(virtual_keyword)
+                } else {
+                    None
+                }
+            }));


The reason why it's not as clean as the mutability check is that in order to build an output::VirtualKeyword we need a reference to CstToIrBuilder (to get the node id), making it difficult to implement the TryFrom trait.

We could implement smarter transformations, but I don't think is needed for now.

Is that a requirement?
I wonder why does this keyword get a NodeId and the bunch of utilities that come with it, compared to other attributes that we only record their existence/absence (FunctionVisibility or FunctionMutability). AFAICT, it has no other associated metadata to record.

I'm not sure, maybe @ggiraldez can chime in here. I'll go ahead and merge since I don't think it's related to the PR itself.

FunctionVisibility and FunctionMutability are "synthetic" enums, that's why they don't register any traceability back to the CST. The main reason is that in some (most?) cases we want a default value, even in the absence of a language attribute. Eg. function mutability is NonPayable by default, but there's no associated attribute; or free functions have Internal visibility, but they cannot have an explicit visibility attribute.

As to why this attribute records the NodeId and other information, I'm a bit fuzzy on the details, but the change happened in #1751 and I think one of the reasons (maybe the main reason) was to allow more precise diagnostics that may need to reference the keywords. This is actually a shortcoming for visibility/mutability in that area.

Thanks for the context.
So, if validation happens at this stage (where we have the IR text ranges), we might be able to simplify the AST for something like this to have just an is_virtual: bool property.

There's no way to add a boolean field type to the IR model right now. Previously that was automatic with unique terminals used as fields, but as part of the change mentioned above we changed it to hold the terminal.

There are a couple of cases where this happens in the language: abstract keyword for contracts and indexed keyword for event parameters (off the top of my head, I may be missing some).

We could change the representation of sequence fields of unique terminals back to a bool, or we can add it as a feature to the IR model mutator, to selectively do it for some fields. The second alternative would be preferable if we want to customize the name of the field, eg. to is_virtual, as otherwise it would be derived from the terminal name, so that would be virtual_keyword: bool in this case.

If we take the mutator feature route, we can probably use the synthetic terminal type External for that, and generate a bool field in those cases.

github-actions · 2026-06-05T17:07:56Z

Bencher Report

Branch	teofr/diag-multiple-virtual
Testbed	ci

⚠️ WARNING: Truncated view!
The full continuous benchmarking report exceeds the maximum length allowed on this platform.

⚠️ WARNING: No Threshold found!
Without a Threshold, no Alerts will ever be generated.

🚨 3 Alerts

🐰 View full continuous benchmarking report in Bencher

OmarTawfik

Left one question.

OmarTawfik

Left one question.

feat(v2): diagnose duplicate virtual specifiers

1f53a14

Reject functions, fallback/receive functions, and modifiers that carry more than one `virtual` keyword, matching solc's ParserError 6879/2662. Closes the `virtual` part of the SDR[1730] validation TODO in the IR builder.

teofr requested a review from Copilot June 5, 2026 16:57

Copilot started reviewing on behalf of teofr June 5, 2026 16:57 View session

Copilot AI reviewed Jun 5, 2026

View reviewed changes

teofr commented Jun 5, 2026

View reviewed changes

teofr marked this pull request as ready for review June 5, 2026 17:07

teofr requested review from a team as code owners June 5, 2026 17:07

OmarTawfik reviewed Jun 5, 2026

View reviewed changes

OmarTawfik approved these changes Jun 5, 2026

View reviewed changes

teofr added this pull request to the merge queue Jun 8, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 8, 2026

teofr added this pull request to the merge queue Jun 8, 2026

Merged via the queue into main with commit de57ced Jun 8, 2026
18 checks passed

teofr deleted the teofr/diag-multiple-virtual branch June 8, 2026 09:04

Conversation

teofr commented Jun 5, 2026

Uh oh!

changeset-bot Bot commented Jun 5, 2026

⚠️ No Changeset found

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

teofr Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teofr Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

ggiraldez Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

ggiraldez Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 5, 2026

Bencher Report

⚠️ WARNING: Truncated view!

⚠️ WARNING: No Threshold found!

🚨 3 Alerts

Uh oh!

OmarTawfik left a comment

Choose a reason for hiding this comment

Uh oh!

OmarTawfik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

teofr Jun 5, 2026 •

edited

Loading

OmarTawfik Jun 5, 2026 •

edited

Loading