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

[WIP] Experiment: stabilize never type #79470

Conversation

nikomatsakis
Copy link
Contributor

Follow-up to #79366 for the purposes of running crater

We used to avoid doing this because we didn't want to make coercion depend on
the state of inference. For better or worse, we have moved away from this
position over time. Therefore, I am going to go ahead and resolve the `b`
target type early on so that it is done uniformly.

(The older technique for managing this was always something of a hack
regardless; if we really wanted to avoid integrating coercion and inference we
needed to be more disciplined about it.)
I didn't like the sub-unify code executing when a predicate was
ENQUEUED, that felt fragile. I would have preferred to move the
sub-unify code so that it only occurred during generalization, but
that impacted diagnostics, so having it also occur when we process
subtype predicates felt pretty reasonable. (I guess we only need one
or the other, but I kind of prefer both, since the generalizer
ultimately feels like the *right* place to guarantee the properties we
want.)
Motivation: in upcoming commits, we are going to create a graph of the
coercion relationships between variables. We want to
distinguish *coercion* specifically from other sorts of subtyping, as
it indicates values flowing from one place to another via assignment.
Along the way, simplify and document the logic more clearly.
We now fallback type variables using the following rules:

* Construct a coercion graph `A -> B` where `A` and `B` are unresolved
  type variables or the `!` type.
* Let D be those variables that are reachable from `!`.
* Let N be those variables that are reachable from a variable not in
D.
* All variables in (D \ N) fallback to `!`.
* All variables in (D & N) fallback to `()`.
The comment seems incorrect. Testing revealed that the examples in
question still work (as well as some variants) even without the
special casing here.
Instead, we now record those type variables that are the target of a
`NeverToAny` adjustment and consider those to be the "diverging" type
variables. This allows us to remove the special case logic that
creates a type variable for `!` in coercion.
Instead, we now rely on the code that looks for a NeverToAny adjustment.
This could cause us to visit the start node twice.
Extend the `DepthFirstSearch` iterator so that it can be re-used and
extended with add'l start nodes. Then replace the FxHashSets of nodes
we were using in the fallback analysis with a single iterator. This
way we won't re-walk portions of the graph that are reached more than
once, and we also do less allocation etc.
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 27, 2020
@nikomatsakis
Copy link
Contributor Author

@bors try

@nikomatsakis
Copy link
Contributor Author

r? @ghost

@bors
Copy link
Contributor

bors commented Nov 27, 2020

⌛ Trying commit 5ef3eb6 with merge 449cf216550e3ee885a40c684da555a6ab31a065...

@bors
Copy link
Contributor

bors commented Nov 27, 2020

☀️ Try build successful - checks-actions
Build commit: 449cf216550e3ee885a40c684da555a6ab31a065 (449cf216550e3ee885a40c684da555a6ab31a065)

_FC(X),
_FD(X),
_FE(X),
_FF(X),

V3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be fixing rustfmt here or telling rustfmt to skip this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you'll want to post any comments on #79366 as this PR is just for the purpose of running crater

@oli-obk
Copy link
Contributor

oli-obk commented Nov 28, 2020

I would start crater, but I don't know if just a check suffices or if we also need to run all tests

@nikomatsakis
Copy link
Contributor Author

@craterbot build-and-test

check would probably suffice but it is possible for these changes to change the semantics of running code and so forth.

@craterbot
Copy link
Collaborator

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 29, 2020
@nikomatsakis
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Nov 30, 2020

⌛ Trying commit 3ad1e4d with merge b2f80f6008cdafc35b4472bd46afa16cc6b0cd63...

@bors
Copy link
Contributor

bors commented Nov 30, 2020

☀️ Try build successful - checks-actions
Build commit: b2f80f6008cdafc35b4472bd46afa16cc6b0cd63 (b2f80f6008cdafc35b4472bd46afa16cc6b0cd63)

@nikomatsakis
Copy link
Contributor Author

@craterbot check

I'm going to start with a check build and we can consider a run-and-test check later.

@craterbot
Copy link
Collaborator

👌 Experiment pr-79470 created and queued.
🤖 Automatically detected try build b2f80f6008cdafc35b4472bd46afa16cc6b0cd63
⚠️ Try build based on commit b7ebc6b, but latest commit is 3ad1e4d. Did you forget to make a new try build?
🔍 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 craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2020
@nikomatsakis
Copy link
Contributor Author

Seems like it'll be a while :)

@craterbot
Copy link
Collaborator

🚧 Experiment pr-79470 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-79470 is completed!
📊 119 regressed and 8 fixed (132594 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

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Dec 14, 2020
@nikomatsakis
Copy link
Contributor Author

Relatively few root failures, but definitely some impact. I did a bit of digging but will have to read in more carefully.

One sample error (chmlib v1.0.0):

[INFO] [stdout] error[E0277]: the trait bound `Continuation: From<!>` is not satisfied
[INFO] [stdout]    --> src/lib.rs:485:13
[INFO] [stdout]     |
[INFO] [stdout] 485 |         chm.for_each(Filter::all(), |_, _| panic!("Oops..."))
[INFO] [stdout]     |             ^^^^^^^^ the trait `From<!>` is not implemented for `Continuation`
[INFO] [stdout]     |
[INFO] [stdout]     = help: the following implementations were found:
[INFO] [stdout]               <Continuation as From<()>>
[INFO] [stdout]               <Continuation as From<std::result::Result<(), E>>>
[INFO] [stdout]     = note: required because of the requirements on the impl of `Into<Continuation>` for `!`

@crlf0710 crlf0710 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 18, 2020
@tema3210
Copy link

AFAIK there are three root failures: "trait is not implemented for !", "never type was propageted as a generic param to smth." and type inference failures.

  • The first point was discussed before
  • Second is the major problem: tainting generic parameters. For example: here the anonymous future type got a never return type (and caused error). The point is that these error are caused by proper usage of never type: regressed futures used a loop, which is an expression of type never. I guess to fix this we have either to invent some crazy schema or to break part of a world in the next edition.
  • An inference failure only happened here.

@crlf0710 crlf0710 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Jan 15, 2021
@crlf0710 crlf0710 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants