Skip to content

Fix the static_functions_not_mutating error messsage #78052

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

felixinkinto
Copy link

@felixinkinto felixinkinto commented Dec 9, 2024

Fix #77835

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! I left a comment inline about the text and as a separate note: please modify existing tests (because the text is now different) or make sure that we have tests for both nonmutating and mutating attributes that exercise the new diagnostics before we can run CI and land this.

@@ -1718,7 +1718,7 @@ ERROR(functions_mutating_and_not,none,
"method must not be declared both %0 and %1",
(SelfAccessKind, SelfAccessKind))
ERROR(static_functions_not_mutating,none,
"static functions must not be declared mutating", ())
"nonmutating or mutating modifiers cannot be used on static functions because they do not operate on instance properties or self", ())
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of clamping mutating and nonmutating together it would be better to print one or the other depending on what the attribute was, a boolean flag could be used to distinguish between the two (i.e. no_overloads_match_exactly_in_call does a similar thing reference vs. call).

Instead of "operate" we can use "cannot access" and the "because" part can be turned into a note.

Copy link
Author

Choose a reason for hiding this comment

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

@xedin
Thanks for the advice!
I fixed based on your suggestion.

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

I left one more small note inline. We also have a few tests that use the diagnostic and they'd need to be adjusted before we can land this:

test/Sema/immutability.swift:134
test/decl/subscript/static.swift:23
test/decl/subscript/static.swift:24
test/decl/overload.swift:413

@AnthonyLatsis
Copy link
Collaborator

@felixinkinto Next time, please ask us to assign you to an issue before you start working on one. A good first issue is a queue, not a race. We want to avoid instances of multiple people independently working on them.

@felixinkinto
Copy link
Author

@xedin sorry for the delay for long. I addressed the feedback. May I ask some questions to do behaviour checks?

  1. Does the order matter? I realized note usually follows the error in other areas.
➜  test xcrun -sdk macosx build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/bin/swiftc test.swift
test.swift:4:3: error: 'nonmutating' modifier cannot be used on static functions
2 |
3 | struct S {
4 |   nonmutating static func foo() {}
  |   |                       `- note: static members cannot access instance properties or self
  |   `- error: 'nonmutating' modifier cannot be used on static functions
5 |
6 |
  1. After testing the locally built compiler in Xcode according to this manual, it seems that the existing swiftc is overriding the errors in the IDE.
    Screenshot 2025-03-07 at 10 22 06 this build log shows my local compiler.

Screenshot 2025-03-07 at 10 22 17 this is still the default swiftc

@felixinkinto
Copy link
Author

felixinkinto commented Mar 7, 2025

@felixinkinto Next time, please ask us to assign you to an issue before you start working on one. A good first issue is a queue, not a race. We want to avoid instances of multiple people independently working on them.

@AnthonyLatsis
sorry I’ll make sure to ask for an assignment before starting work on an issue next time.

@felixinkinto felixinkinto requested a review from xedin March 7, 2025 01:33
@felixinkinto
Copy link
Author

@swift-ci please test Windows platform

@felixinkinto felixinkinto marked this pull request as draft April 22, 2025 01:31
@felixinkinto felixinkinto marked this pull request as ready for review April 22, 2025 01:31
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.

Diagnostic for static nonmutating should be improved
3 participants