Skip to content

Disentangle InferCtxt, MemCategorizationContext and ExprUseVisitor. #42563

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

Merged
merged 6 commits into from
Jun 10, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jun 9, 2017

At some point in the past, InferCtxt started being used to replace an old "Typer" abstraction, which provided access to TypeckTables and had optionally type inference to account for.
That didn't play so nicely with the 'gcx/'tcx split and I had to introduce borrowck_fake_infer_ctxt.
The situation wasn't great but it wasn't too painful inside rustc itself.

Recently I've found that method being used in clippy, which does need EUV (before we make it plausible to run lints on HAIR or MIR), and set out to separate inference from tables, for the sake of lint authors.
Also fixes #42435 to make it trivial to compute type layout or use EUV from lints.

The remaining uses of TypeckTables in InferCtxt are for closure kinds and signatures, used in trait selection and projection normalization. The solution there is likely to add them as bounds to ParamEnv.

r? @nikomatsakis
cc @mcarton @llogiq @Manishearth

@eddyb eddyb requested a review from nikomatsakis June 9, 2017 14:32
@eddyb eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jun 9, 2017
@eddyb
Copy link
Member Author

eddyb commented Jun 9, 2017

cc @Mark-Simulacrum @tomprince This could use some cratering/cargobombing.

EDIT: Although, if crater works again, I could do that now myself.

@llogiq
Copy link
Contributor

llogiq commented Jun 9, 2017

Note that some lints use visitors because they need a second pass (e.g. to find usages of something). Those Lint's could be rewritten to store the relevant information, thus reducing them to single-pass, but this isn't exactly trivial.

@eddyb
Copy link
Member Author

eddyb commented Jun 9, 2017

@llogiq ExprUseVisitor is not the same as a HIR visitor, it's a tool to process all borrows/moves.

EDIT: as in, it's not feasible to replace it without replicating large portions of it.
Only HAIR & MIR have the same level of detail necessary for replacing MC/EUV completely.

@eddyb
Copy link
Member Author

eddyb commented Jun 10, 2017

Crater report only has network errors and clippy (gotta break it to make it better 😂).

@eddyb eddyb 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 Jun 10, 2017
@tomprince
Copy link
Member

@bors try

bors added a commit that referenced this pull request Jun 10, 2017
Disentangle InferCtxt, MemCategorizationContext and ExprUseVisitor.

At some point in the past, `InferCtxt` started being used to replace an old "`Typer`" abstraction, which provided access to `TypeckTables` and had optionally type inference to account for.
That didn't play so nicely with the `'gcx`/`'tcx` split and I had to introduce `borrowck_fake_infer_ctxt`.
The situation wasn't great but it wasn't too painful inside `rustc` itself.

Recently I've found that method being used in clippy, which does need EUV (before we make it plausible to run lints on HAIR or MIR), and set out to separate inference from tables, for the sake of lint authors.
Also fixes #42435 to make it trivial to compute type layout or use EUV from lints.

The remaining uses of `TypeckTables` in `InferCtxt` are for closure kinds and signatures, used in trait selection and projection normalization. The solution there is likely to add them as bounds to `ParamEnv`.

r? @nikomatsakis
cc @mcarton @llogiq @Manishearth
@bors
Copy link
Collaborator

bors commented Jun 10, 2017

⌛ Trying commit 9685a81 with merge 655a47b...

self.infcx.is_tainted_by_errors()
}

fn closure_kind(&self, def_id: DefId, span: Span) -> ty::ClosureKind {
Copy link
Contributor

Choose a reason for hiding this comment

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

did this get copied from the version in infcx? I don't see any corresponding - lines...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but it works a bit differently - ideally in_progress_tables would be used nowhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I might be able to make it simpler and check tables then tcx without caring about infcx, since now I am always inserting the kinds in the tables ahead of time.

@nikomatsakis
Copy link
Contributor

r=me modulo crater etc

@bors
Copy link
Collaborator

bors commented Jun 10, 2017

💥 Test timed out

@eddyb
Copy link
Member Author

eddyb commented Jun 10, 2017

Second crater report doesn't have any real regressions either.

@eddyb
Copy link
Member Author

eddyb commented Jun 10, 2017

@bors r=nikomatsakis

@bors
Copy link
Collaborator

bors commented Jun 10, 2017

📌 Commit fc5c31c has been approved by nikomatsakis

@bors
Copy link
Collaborator

bors commented Jun 10, 2017

⌛ Testing commit fc5c31c with merge b7613f8...

bors added a commit that referenced this pull request Jun 10, 2017
Disentangle InferCtxt, MemCategorizationContext and ExprUseVisitor.

At some point in the past, `InferCtxt` started being used to replace an old "`Typer`" abstraction, which provided access to `TypeckTables` and had optionally type inference to account for.
That didn't play so nicely with the `'gcx`/`'tcx` split and I had to introduce `borrowck_fake_infer_ctxt`.
The situation wasn't great but it wasn't too painful inside `rustc` itself.

Recently I've found that method being used in clippy, which does need EUV (before we make it plausible to run lints on HAIR or MIR), and set out to separate inference from tables, for the sake of lint authors.
Also fixes #42435 to make it trivial to compute type layout or use EUV from lints.

The remaining uses of `TypeckTables` in `InferCtxt` are for closure kinds and signatures, used in trait selection and projection normalization. The solution there is likely to add them as bounds to `ParamEnv`.

r? @nikomatsakis
cc @mcarton @llogiq @Manishearth
@bors
Copy link
Collaborator

bors commented Jun 10, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing b7613f8 to master...

@bors bors merged commit fc5c31c into rust-lang:master Jun 10, 2017
@eddyb eddyb deleted the infer branch June 10, 2017 21:37
@tomprince
Copy link
Member

I let the cargobomb run I started complete, and the results are here.

@eddyb
Copy link
Member Author

eddyb commented Jun 13, 2017

@tomprince Thanks! Those all look like false positives, so I'm confident this is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Track ParamEnv in LateContext.
5 participants