Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Diagnose var in interfaces #4907

Merged
merged 2 commits into from
Feb 6, 2025
Merged

Conversation

geoffromer
Copy link
Contributor

Previously this was ignored, and then #4720 accidentally made it a crash bug

Previously this was ignored, and then carbon-language#4720 accidentally made it a crash bug
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Good catch!

@jonmeow jonmeow enabled auto-merge February 6, 2025 19:02
@@ -386,6 +386,12 @@ auto HandleParseNode(Context& context, Parse::VariableDeclId node_id) -> bool {
}
return true;
}
if (context.GetCurrentScopeAs<SemIR::InterfaceDecl>()) {
CARBON_DIAGNOSTIC(VarInInterfaceDecl, Error,
"interfaces cannot have `var` members");
Copy link
Contributor

Choose a reason for hiding this comment

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

The style guide wants to say what occurred, rather than what cannot occur: https://github.com/carbon-language/carbon-lang/blob/40f3de2c0726983c7c41200eddd479ceec00be5e/toolchain/docs/diagnostics.md#diagnostic-message-style-guide

Maybe "var binding found in interface"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I wasn't thinking about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, done.

@jonmeow jonmeow disabled auto-merge February 6, 2025 19:09
Copy link
Contributor

@danakj danakj left a comment

Choose a reason for hiding this comment

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

LGTM too, thanks

@danakj danakj enabled auto-merge February 6, 2025 19:13
@danakj danakj added this pull request to the merge queue Feb 6, 2025
Merged via the queue into carbon-language:trunk with commit 55714dd Feb 6, 2025
8 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants