Skip to content
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

Change error diagnosed for when self isn't available in scope #59140

Merged
merged 10 commits into from Jun 3, 2022
Merged

Change error diagnosed for when self isn't available in scope #59140

merged 10 commits into from Jun 3, 2022

Conversation

NSAntoine
Copy link
Contributor

As discussed on https://forums.swift.org/t/changing-error-diagnosed-for-when-self-isn-t-available/57616, the purpose of these changes are to hopefully make it more obvious, especially to beginner programmers, what the purpose of self is, as these changes will diagnose cannot find self in scope, did you mean to declare or extend a type rather than just cannot find self in scope when self isn't available.

@NSAntoine NSAntoine marked this pull request as draft May 28, 2022 10:12
@NSAntoine
Copy link
Contributor Author

@AnthonyLatsis test?

@hamishknight
Copy link
Collaborator

@swift-ci please test

@NSAntoine
Copy link
Contributor Author

@hamishknight Fixed compilation errors, try to test it again, also, every test file should have // RUN: %target-typecheck-verify-swift at the top, right?

Copy link
Collaborator

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

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

Nice!

include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved
lib/Sema/PreCheckExpr.cpp Outdated Show resolved Hide resolved
@hamishknight
Copy link
Collaborator

Every test file should have // RUN: %target-typecheck-verify-swift at the top, right?

Yup, every test that needs to run a typecheck over the test file

NSAntoine and others added 2 commits May 28, 2022 15:42
Co-authored-by: Hamish Knight <hamish_github@mediocremail.com>
…he string to `self`

Co-authored-by: Hamish Knight <hamish_github@mediocremail.com>
@hamishknight
Copy link
Collaborator

@swift-ci please test

include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved
test/Syntax/self_cannot_be_found.swift Outdated Show resolved Hide resolved
lib/Sema/PreCheckExpr.cpp Outdated Show resolved Hide resolved
@NSAntoine
Copy link
Contributor Author

@AnthonyLatsis addressed all concerns, could you try test now?

@AnthonyLatsis
Copy link
Collaborator

@swift-ci please smoke test

@NSAntoine
Copy link
Contributor Author

@AnthonyLatsis could you ask it to build a toolchain aswell? The tests pass and everything works but I’m just curious to try out locally

@AnthonyLatsis
Copy link
Collaborator

@swift-ci please build toolchain macOS

@NSAntoine NSAntoine marked this pull request as ready for review May 28, 2022 17:59
@NSAntoine
Copy link
Contributor Author

@xwu Ready for merge now?

@AnthonyLatsis
Copy link
Collaborator

Toolchain: https://ci.swift.org/view/all/job/swift-PR-toolchain-macos/179/

Copy link
Member

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

Looks good, thank you!

@xedin
Copy link
Member

xedin commented Jun 2, 2022

@SerenaKit You might want to consider a tailored diagnostic like this for Self as well.

@xedin
Copy link
Member

xedin commented Jun 2, 2022

@swift-ci please test

@AnthonyLatsis
Copy link
Collaborator

@SerenaKit I'm going to squash this so that we don't pollute the commit history with review-specific amendments.

@AnthonyLatsis AnthonyLatsis merged commit 99116c9 into apple:main Jun 3, 2022
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.

None yet

5 participants