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

Symbol cleanups #110175

Merged
merged 4 commits into from
Apr 12, 2023
Merged

Symbol cleanups #110175

merged 4 commits into from
Apr 12, 2023

Conversation

nnethercote
Copy link
Contributor

There's a bad pattern matching confusion present in this function.
`_anon` gets assigned to, and then `_anon` is used as an unbound
variable in the pattern, which is unrelated to the first `_anon`.
If the `_anon` didn't start with `_` the compiler would give warnings.

This was introduced in rust-lang#104239.

I have rewritten the function to remove the confusion and preserve the
existing behaviour. This seems safest, because the original intent is
not clear.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 11, 2023
@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 11, 2023

📌 Commit 7975779 has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2023
match self {
RegionCtxt::Unknown => 1,
RegionCtxt::Existential(None) => 2,
RegionCtxt::Existential(Some(_anon)) | RegionCtxt::Free(_anon) => 2,
RegionCtxt::Existential(Some(_)) | RegionCtxt::Free(_) => 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching the mistake here, but using _ here is not correct I believe. This should match for the anon symbol inside RegionCtxt::Existential or RegionCtxt::Free.

Copy link
Contributor Author

@nnethercote nnethercote Apr 11, 2023

Choose a reason for hiding this comment

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

I just tried doing what you suggested and then I ran all the tests and they passed. So it doesn't seem to make much difference. In what case might this matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this code is only used for sccs_info, which is used to give information on region vars (for debugging purposes only currently). I think we should prefer to use actual region names (if available, otherwise they are anon) over region information through Location or TyContext (though whether these are really more informative is debatable anyway I guess).

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Apr 11, 2023
…kh726

Symbol cleanups

r? `@jackh726`

cc `@b-naber`
@bjorn3
Copy link
Member

bjorn3 commented Apr 11, 2023

I did something similar in the past, but it seems there have been new cases of Symbol::intern introduced since. https://github.com/rust-lang/rust/pulls?q=is%3Amerged+is%3Apr+author%3Abjorn3+pre-interned+symbol

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Apr 11, 2023
…kh726

Symbol cleanups

r? `@jackh726`

cc `@b-naber`
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Apr 12, 2023
…kh726

Symbol cleanups

r? ``@jackh726``

cc ``@b-naber``
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 12, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#110153 (Fix typos in compiler)
 - rust-lang#110165 (rustdoc: use CSS `overscroll-behavior` instead of JavaScript)
 - rust-lang#110175 (Symbol cleanups)
 - rust-lang#110203 (Remove `..` from return type notation)
 - rust-lang#110205 (rustdoc: make settings radio and checks thicker, less contrast)
 - rust-lang#110222 (Improve the error message when forwarding a matched fragment to another macro)
 - rust-lang#110237 (Split out a separate feature gate for impl trait in associated types)
 - rust-lang#110241 (tidy: Issue an error when UI test limits are too high)

Failed merges:

 - rust-lang#110218 (Remove `ToRegionVid`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b4734f0 into rust-lang:master Apr 12, 2023
@rustbot rustbot added this to the 1.70.0 milestone Apr 12, 2023
@nnethercote nnethercote deleted the symbol-cleanups branch April 13, 2023 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants