Hygiene 2.0: Avoid comparing fields by name #49718
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @eddyb |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #49154) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Updated with some tests. |
|
@bors r+ |
|
📌 Commit 8b2fcb0 has been approved by |
bors
added
S-waiting-on-bors
|
☔ The latest upstream changes (presumably #49875) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
Resolve them into field indices once and then use those resolutions + Fix rebase
|
@bors r=eddyb |
|
📌 Commit fcf4852 has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=1 Looks relatively conflict prone and is blocking other work. |
bors
added a commit
that referenced
this pull request
Apr 13, 2018
Hygiene 2.0: Avoid comparing fields by name
There are two separate commits here (not counting tests):
- The first one unifies named (`obj.name`) and numeric (`obj.0`) field access expressions in AST and HIR. Before field references in these expressions are resolved it doesn't matter whether the field is named or numeric (it's just a symbol) and 99% of code is common. After field references are resolved we work with
them by index for all fields (see the second commit), so it's again not important whether the field was named or numeric (this includes MIR where all fields were already by index).
(This refactoring actually fixed some bugs in HIR-based borrow checker where borrows through names (`S {
0: ref x }`) and indices (`&s.0`) weren't considered overlapping.)
- The second commit removes all by-name field comparison and instead resolves field references to their indices once, and then uses those resolutions. (There are still a few name comparisons in save-analysis, because save-analysis is weird, but they are made correctly hygienic).
Thus we are fixing a bunch of "secondary" field hygiene bugs (in borrow checker, lints).
Fixes #46314
|
☀️ Test successful - status-appveyor, status-travis |
kennytm-githubbot
added a commit
to rust-lang-nursery/rust-toolstate
that referenced
this pull request
Apr 13, 2018
Tested on commit rust-lang/rust@defcfe7. Direct link to PR: <rust-lang/rust#49718> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk).
|
Whoops, I missed this until just now - it breaks Rustfmt as well as Clippy. But I wanted to comment on the strategy here, it seems that this is the kind of thing that should be done on the HIR, but not on the AST - it is useful for tools to be able to tell the two operations apart syntactically and the rest of the compiler could still unify code because it operates on the HIR. The lowering step could easily unify the two kinds of field access. |
|
This is especially annoying for Rustfmt because formatting changes - we can output |
|
For the compiler itself it's more convenient to keep all fields as identifiers even in AST, there are no places where they would be treated differently + parsing logic and hygiene-keeping is simplified, so I optimized for that. |
kngwyu
added a commit
to kngwyu/racer-nightly
that referenced
this pull request
Apr 15, 2018
This update includes removal of ExprKind::TupField(rust-lang/rust#49718). With this change, I changed the logic of method completion for tuple struct and added a related test.
Closed
kngwyu
added a commit
to kngwyu/racer-nightly
that referenced
this pull request
May 18, 2018
By rust-lang/rust#49718, ExprKind::TupField was removed. This commit includes a temorary fix and a tests for this removal.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are two separate commits here (not counting tests):
obj.name) and numeric (obj.0) field access expressions in AST and HIR. Before field references in these expressions are resolved it doesn't matter whether the field is named or numeric (it's just a symbol) and 99% of code is common. After field references are resolved we work withthem by index for all fields (see the second commit), so it's again not important whether the field was named or numeric (this includes MIR where all fields were already by index).
(This refactoring actually fixed some bugs in HIR-based borrow checker where borrows through names (
S { 0: ref x }) and indices (&s.0) weren't considered overlapping.)Thus we are fixing a bunch of "secondary" field hygiene bugs (in borrow checker, lints).
Fixes #46314