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

High priority resolutions for associated variants #57501

Merged
merged 1 commit into from Jan 19, 2019

Conversation

Projects
None yet
8 participants
@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 10, 2019

In #56225 variants were assigned lowest priority during name resolution to avoid crater run and potential breakage.

This PR changes the rules to give variants highest priority instead.
Some motivation:

  • If variants (and their constructors) are treated as associated items, then they are obviously inherent associated items since they don't come from traits.
  • Inherent associated items have higher priority during resolution than associated items from traits.
  • The reason is that there is a way to disambiguate in favor of trait items (<Type as Trait>::Ambiguous), but there's no way to disambiguate in favor of inherent items, so they became unusable in case of ambiguities if they have low priority.
  • It's technically problematic to fallback from associated types to anything until lazy normalization (?) is implemented.

Crater found some regressions from this change, but they are all in type positions, e.g.

fn f() -> Self::Ambiguos { ... } // Variant `Ambiguous` or associated type `Ambiguous`?

, so variants are not usable there right now, but they may become usable in the future if rust-lang/rfcs#2593 is accepted.
This PR keeps code like this successfully resolving, but introduces a future-compatibility lint ambiguous_associated_items that recommends rewriting it as <Self as Trait>::Ambiguous.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 10, 2019

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 10, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

⌛️ Trying commit dcaebe0 with merge 5f78915...

bors added a commit that referenced this pull request Jan 10, 2019

Auto merge of #57501 - petrochenkov:highvar, r=<try>
[WIP] High priority resolutions for associated variants

cc #56225 (comment)
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 10, 2019

@alexreg
Copy link
Contributor

alexreg left a comment

Looks reasonable to me, apart from the little things I mentioned above.

Show resolved Hide resolved src/librustc/lint/builtin.rs Outdated
Show resolved Hide resolved src/librustc_typeck/lib.rs Outdated
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 11, 2019

☀️ Test successful - checks-travis
State: approved= try=True

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 11, 2019

@craterbot run start=master#6ecad338381cc3b8d56e2df22e5971a598eddd6c end=try#5f789154f114767b2d0cb9c5b6c592cb3a635e6f mode=check-only

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 11, 2019

👌 Experiment pr-57501 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 11, 2019

🚧 Experiment pr-57501 is now running on agent aws-1.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 12, 2019

🎉 Experiment pr-57501 is completed!
📊 15 regressed and 0 fixed (50551 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Jan 12, 2019

@petrochenkov Looks like there are some legitimate regressions... not too many, thankfully, and no big crates. Maybe you want to open issues/PRs on them then get this PR in ASAP? This error will make backwards-compatibility for a variants-as-types implementation a lot easier too.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 12, 2019

All the regressions are from variants used in type positions where no true ambiguity exists until variant types are implemented.
It's also good that the number of root regressions is small despite hundreds of non-root regressions.

So, I think we can keep the old behavior and emit a future-compatibility warning in type positions, and just give variants high priority in other contexts.

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Jan 12, 2019

Sounds reasonable. I'll leave you to do that. Feel free to r? me when you're ready. (I should have rights now.)

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 12, 2019

@rust-highfive rust-highfive assigned alexreg and unassigned davidtwco Jan 12, 2019

@petrochenkov petrochenkov force-pushed the petrochenkov:highvar branch from dcaebe0 to 6d9a8f5 Jan 14, 2019

@petrochenkov petrochenkov force-pushed the petrochenkov:highvar branch from 6d9a8f5 to 83b6742 Jan 15, 2019

@petrochenkov petrochenkov changed the title [WIP] High priority resolutions for associated variants High priority resolutions for associated variants Jan 15, 2019

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 16, 2019

☔️ The latest upstream changes (presumably #57321) made this pull request unmergeable. Please resolve the merge conflicts.

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Jan 16, 2019

@petrochenkov Ugh, looks like a rebase is necessary. Maybe wait until the tree is open again? I'm not sure how to tell though.

@petrochenkov petrochenkov force-pushed the petrochenkov:highvar branch from ade0884 to 01d0ae9 Jan 16, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 16, 2019

@bors r=alexreg

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 16, 2019

📌 Commit 01d0ae9 has been approved by alexreg

Centril added a commit to Centril/rust that referenced this pull request Jan 17, 2019

Rollup merge of rust-lang#57501 - petrochenkov:highvar, r=alexreg
High priority resolutions for associated variants

In rust-lang#56225 variants were assigned lowest priority during name resolution to avoid crater run and potential breakage.

This PR changes the rules to give variants highest priority instead.
Some motivation:
- If variants (and their constructors) are treated as associated items, then they are obviously *inherent* associated items since they don't come from traits.
- Inherent associated items have higher priority during resolution than associated items from traits.
- The reason is that there is a way to disambiguate in favor of trait items (`<Type as Trait>::Ambiguous`), but there's no way to disambiguate in favor of inherent items, so they became unusable in case of ambiguities if they have low priority.
- It's technically problematic to fallback from associated types to anything until lazy normalization (?) is implemented.

Crater found some regressions from this change, but they are all in type positions, e.g.
```rust
fn f() -> Self::Ambiguos { ... } // Variant `Ambiguous` or associated type `Ambiguous`?
```
, so variants are not usable there right now, but they may become usable in the future if rust-lang/rfcs#2593 is accepted.
This PR keeps code like this successfully resolving, but introduces a future-compatibility lint `ambiguous_associated_items` that recommends rewriting it as `<Self as Trait>::Ambiguous`.

Centril added a commit to Centril/rust that referenced this pull request Jan 17, 2019

Rollup merge of rust-lang#57501 - petrochenkov:highvar, r=alexreg
High priority resolutions for associated variants

In rust-lang#56225 variants were assigned lowest priority during name resolution to avoid crater run and potential breakage.

This PR changes the rules to give variants highest priority instead.
Some motivation:
- If variants (and their constructors) are treated as associated items, then they are obviously *inherent* associated items since they don't come from traits.
- Inherent associated items have higher priority during resolution than associated items from traits.
- The reason is that there is a way to disambiguate in favor of trait items (`<Type as Trait>::Ambiguous`), but there's no way to disambiguate in favor of inherent items, so they became unusable in case of ambiguities if they have low priority.
- It's technically problematic to fallback from associated types to anything until lazy normalization (?) is implemented.

Crater found some regressions from this change, but they are all in type positions, e.g.
```rust
fn f() -> Self::Ambiguos { ... } // Variant `Ambiguous` or associated type `Ambiguous`?
```
, so variants are not usable there right now, but they may become usable in the future if rust-lang/rfcs#2593 is accepted.
This PR keeps code like this successfully resolving, but introduces a future-compatibility lint `ambiguous_associated_items` that recommends rewriting it as `<Self as Trait>::Ambiguous`.

Centril added a commit to Centril/rust that referenced this pull request Jan 17, 2019

Rollup merge of rust-lang#57501 - petrochenkov:highvar, r=alexreg
High priority resolutions for associated variants

In rust-lang#56225 variants were assigned lowest priority during name resolution to avoid crater run and potential breakage.

This PR changes the rules to give variants highest priority instead.
Some motivation:
- If variants (and their constructors) are treated as associated items, then they are obviously *inherent* associated items since they don't come from traits.
- Inherent associated items have higher priority during resolution than associated items from traits.
- The reason is that there is a way to disambiguate in favor of trait items (`<Type as Trait>::Ambiguous`), but there's no way to disambiguate in favor of inherent items, so they became unusable in case of ambiguities if they have low priority.
- It's technically problematic to fallback from associated types to anything until lazy normalization (?) is implemented.

Crater found some regressions from this change, but they are all in type positions, e.g.
```rust
fn f() -> Self::Ambiguos { ... } // Variant `Ambiguous` or associated type `Ambiguous`?
```
, so variants are not usable there right now, but they may become usable in the future if rust-lang/rfcs#2593 is accepted.
This PR keeps code like this successfully resolving, but introduces a future-compatibility lint `ambiguous_associated_items` that recommends rewriting it as `<Self as Trait>::Ambiguous`.

bors added a commit that referenced this pull request Jan 17, 2019

Auto merge of #57690 - Centril:rollup, r=Centril
Rollup of 18 pull requests

Successful merges:

 - #56594 (Remove confusing comment about ideally using `!` for `c_void`)
 - #56996 (Move spin_loop_hint to core::hint module)
 - #57065 (Optimize try_mark_green and eliminate the lock on dep node colors)
 - #57107 (Add a regression test for mutating a non-mut #[thread_local])
 - #57253 (Make privacy checking, intrinsic checking and liveness checking incremental)
 - #57268 (Add a target option "merge-functions", and a corresponding -Z flag (works around #57356))
 - #57340 (Use correct tracking issue for c_variadic)
 - #57357 (Cleanup PartialEq docs.)
 - #57370 (Support passing cflags/cxxflags/ldflags to LLVM build)
 - #57501 (High priority resolutions for associated variants)
 - #57551 (resolve: Add a test for issue #57539)
 - #57610 (Fix nested `?` matchers)
 - #57635 (use structured macro and path resolve suggestions)
 - #57636 (Fix sources sidebar not showing up)
 - #57646 (Fixes text becoming invisible when element targetted)
 - #57654 (Add some links in std::fs.)
 - #57655 (OSX: fix #57534 registering thread dtors while running thread dtors)
 - #57659 (Fix release manifest generation)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jan 17, 2019

Auto merge of #57690 - Centril:rollup, r=Centril
Rollup of 18 pull requests

Successful merges:

 - #56594 (Remove confusing comment about ideally using `!` for `c_void`)
 - #56996 (Move spin_loop_hint to core::hint module)
 - #57065 (Optimize try_mark_green and eliminate the lock on dep node colors)
 - #57107 (Add a regression test for mutating a non-mut #[thread_local])
 - #57253 (Make privacy checking, intrinsic checking and liveness checking incremental)
 - #57268 (Add a target option "merge-functions", and a corresponding -Z flag (works around #57356))
 - #57340 (Use correct tracking issue for c_variadic)
 - #57357 (Cleanup PartialEq docs.)
 - #57370 (Support passing cflags/cxxflags/ldflags to LLVM build)
 - #57501 (High priority resolutions for associated variants)
 - #57551 (resolve: Add a test for issue #57539)
 - #57610 (Fix nested `?` matchers)
 - #57635 (use structured macro and path resolve suggestions)
 - #57636 (Fix sources sidebar not showing up)
 - #57646 (Fixes text becoming invisible when element targetted)
 - #57654 (Add some links in std::fs.)
 - #57655 (OSX: fix #57534 registering thread dtors while running thread dtors)
 - #57659 (Fix release manifest generation)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jan 18, 2019

Auto merge of #57690 - Centril:rollup, r=Centril
Rollup of 18 pull requests

Successful merges:

 - #56594 (Remove confusing comment about ideally using `!` for `c_void`)
 - #56996 (Move spin_loop_hint to core::hint module)
 - #57065 (Optimize try_mark_green and eliminate the lock on dep node colors)
 - #57107 (Add a regression test for mutating a non-mut #[thread_local])
 - #57253 (Make privacy checking, intrinsic checking and liveness checking incremental)
 - #57268 (Add a target option "merge-functions", and a corresponding -Z flag (works around #57356))
 - #57340 (Use correct tracking issue for c_variadic)
 - #57357 (Cleanup PartialEq docs.)
 - #57370 (Support passing cflags/cxxflags/ldflags to LLVM build)
 - #57501 (High priority resolutions for associated variants)
 - #57551 (resolve: Add a test for issue #57539)
 - #57610 (Fix nested `?` matchers)
 - #57635 (use structured macro and path resolve suggestions)
 - #57636 (Fix sources sidebar not showing up)
 - #57646 (Fixes text becoming invisible when element targetted)
 - #57654 (Add some links in std::fs.)
 - #57655 (OSX: fix #57534 registering thread dtors while running thread dtors)
 - #57659 (Fix release manifest generation)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jan 18, 2019

Rollup merge of rust-lang#57501 - petrochenkov:highvar, r=alexreg
High priority resolutions for associated variants

In rust-lang#56225 variants were assigned lowest priority during name resolution to avoid crater run and potential breakage.

This PR changes the rules to give variants highest priority instead.
Some motivation:
- If variants (and their constructors) are treated as associated items, then they are obviously *inherent* associated items since they don't come from traits.
- Inherent associated items have higher priority during resolution than associated items from traits.
- The reason is that there is a way to disambiguate in favor of trait items (`<Type as Trait>::Ambiguous`), but there's no way to disambiguate in favor of inherent items, so they became unusable in case of ambiguities if they have low priority.
- It's technically problematic to fallback from associated types to anything until lazy normalization (?) is implemented.

Crater found some regressions from this change, but they are all in type positions, e.g.
```rust
fn f() -> Self::Ambiguos { ... } // Variant `Ambiguous` or associated type `Ambiguous`?
```
, so variants are not usable there right now, but they may become usable in the future if rust-lang/rfcs#2593 is accepted.
This PR keeps code like this successfully resolving, but introduces a future-compatibility lint `ambiguous_associated_items` that recommends rewriting it as `<Self as Trait>::Ambiguous`.

bors added a commit that referenced this pull request Jan 18, 2019

Auto merge of #57727 - Centril:rollup, r=Centril
Rollup of 22 pull requests

Successful merges:

 - #56594 (Remove confusing comment about ideally using `!` for `c_void`)
 - #57107 (Add a regression test for mutating a non-mut #[thread_local])
 - #57340 (Use correct tracking issue for c_variadic)
 - #57357 (Cleanup PartialEq docs.)
 - #57370 (Support passing cflags/cxxflags/ldflags to LLVM build)
 - #57475 (Add signed num::NonZeroI* types)
 - #57501 (High priority resolutions for associated variants)
 - #57551 (resolve: Add a test for issue #57539)
 - #57610 (Fix nested `?` matchers)
 - #57634 (Remove an unused function argument)
 - #57635 (use structured macro and path resolve suggestions)
 - #57636 (Fix sources sidebar not showing up)
 - #57646 (Fixes text becoming invisible when element targetted)
 - #57649 (privacy: Account for associated existential types)
 - #57650 (librustc_metadata: Pass a default value when unwrapping a span)
 - #57654 (Add some links in std::fs.)
 - #57658 (Two HIR tweaks)
 - #57659 (Fix release manifest generation)
 - #57683 (Document Unpin in std::prelude documentation)
 - #57685 (Enhance `Pin` impl applicability for `PartialEq` and `PartialOrd`.)
 - #57698 (Fix typo bug in DepGraph::try_mark_green().)
 - #57720 (Fix suggestions given mulitple bad lifetimes)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jan 18, 2019

Rollup merge of rust-lang#57501 - petrochenkov:highvar, r=alexreg
High priority resolutions for associated variants

In rust-lang#56225 variants were assigned lowest priority during name resolution to avoid crater run and potential breakage.

This PR changes the rules to give variants highest priority instead.
Some motivation:
- If variants (and their constructors) are treated as associated items, then they are obviously *inherent* associated items since they don't come from traits.
- Inherent associated items have higher priority during resolution than associated items from traits.
- The reason is that there is a way to disambiguate in favor of trait items (`<Type as Trait>::Ambiguous`), but there's no way to disambiguate in favor of inherent items, so they became unusable in case of ambiguities if they have low priority.
- It's technically problematic to fallback from associated types to anything until lazy normalization (?) is implemented.

Crater found some regressions from this change, but they are all in type positions, e.g.
```rust
fn f() -> Self::Ambiguos { ... } // Variant `Ambiguous` or associated type `Ambiguous`?
```
, so variants are not usable there right now, but they may become usable in the future if rust-lang/rfcs#2593 is accepted.
This PR keeps code like this successfully resolving, but introduces a future-compatibility lint `ambiguous_associated_items` that recommends rewriting it as `<Self as Trait>::Ambiguous`.

bors added a commit that referenced this pull request Jan 18, 2019

Auto merge of #57732 - Centril:rollup, r=Centril
Rollup of 21 pull requests

Successful merges:

 - #56594 (Remove confusing comment about ideally using `!` for `c_void`)
 - #57107 (Add a regression test for mutating a non-mut #[thread_local])
 - #57340 (Use correct tracking issue for c_variadic)
 - #57357 (Cleanup PartialEq docs.)
 - #57370 (Support passing cflags/cxxflags/ldflags to LLVM build)
 - #57501 (High priority resolutions for associated variants)
 - #57551 (resolve: Add a test for issue #57539)
 - #57610 (Fix nested `?` matchers)
 - #57634 (Remove an unused function argument)
 - #57635 (use structured macro and path resolve suggestions)
 - #57636 (Fix sources sidebar not showing up)
 - #57646 (Fixes text becoming invisible when element targetted)
 - #57650 (librustc_metadata: Pass a default value when unwrapping a span)
 - #57654 (Add some links in std::fs.)
 - #57658 (Two HIR tweaks)
 - #57659 (Fix release manifest generation)
 - #57683 (Document Unpin in std::prelude documentation)
 - #57685 (Enhance `Pin` impl applicability for `PartialEq` and `PartialOrd`.)
 - #57698 (Fix typo bug in DepGraph::try_mark_green().)
 - #57720 (Fix suggestions given mulitple bad lifetimes)
 - #57725 (Use structured suggestion to surround struct literal with parenthesis)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jan 19, 2019

Rollup merge of rust-lang#57501 - petrochenkov:highvar, r=alexreg
High priority resolutions for associated variants

In rust-lang#56225 variants were assigned lowest priority during name resolution to avoid crater run and potential breakage.

This PR changes the rules to give variants highest priority instead.
Some motivation:
- If variants (and their constructors) are treated as associated items, then they are obviously *inherent* associated items since they don't come from traits.
- Inherent associated items have higher priority during resolution than associated items from traits.
- The reason is that there is a way to disambiguate in favor of trait items (`<Type as Trait>::Ambiguous`), but there's no way to disambiguate in favor of inherent items, so they became unusable in case of ambiguities if they have low priority.
- It's technically problematic to fallback from associated types to anything until lazy normalization (?) is implemented.

Crater found some regressions from this change, but they are all in type positions, e.g.
```rust
fn f() -> Self::Ambiguos { ... } // Variant `Ambiguous` or associated type `Ambiguous`?
```
, so variants are not usable there right now, but they may become usable in the future if rust-lang/rfcs#2593 is accepted.
This PR keeps code like this successfully resolving, but introduces a future-compatibility lint `ambiguous_associated_items` that recommends rewriting it as `<Self as Trait>::Ambiguous`.

bors added a commit that referenced this pull request Jan 19, 2019

Auto merge of #57752 - Centril:rollup, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #57268 (Add a target option "merge-functions", and a corresponding -Z flag (works around #57356))
 - #57476 (Move glob map use to query and get rid of CrateAnalysis)
 - #57501 (High priority resolutions for associated variants)
 - #57573 (Querify `entry_fn`)
 - #57610 (Fix nested `?` matchers)
 - #57634 (Remove an unused function argument)
 - #57653 (Make the contribution doc reference the guide more)
 - #57666 (Generalize `huge-enum.rs` test and expected stderr for more cross platform cases)
 - #57698 (Fix typo bug in DepGraph::try_mark_green().)
 - #57746 (Update README.md)

Failed merges:

r? @ghost

@bors bors merged commit 01d0ae9 into rust-lang:master Jan 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment