Stream: t-compiler

Topic: #51002 kill rustc::cfg consumers


Wesley Wiser (Sep 12 2018 at 14:19, on Zulip):

So I took a stab at rewriting the UnconditionalRecursion lint to operate on MIR instead of the CFG. I've got something that basically works but I want to make sure my approach is reasonable.

To get the mir for a function, I'm using the optimized_mir() query. I've tried using some of the other queries such as mir_built()but since UnconditionalRecursion is a late lint, those other MIR queries' values have already been stolen and are unavailable. I tried making UnconditionalRecursion an early lint instead but I can't find any TyCtxts available to early lints and so I don't know how to get the MIR at all in an early lint context.

Is using the optimized MIR ok or is there something else I should be using?

nikomatsakis (Sep 12 2018 at 16:28, on Zulip):

I .. huh

nikomatsakis (Sep 12 2018 at 16:28, on Zulip):

this feels like something that should use unoptimized MIR to me

nikomatsakis (Sep 12 2018 at 16:28, on Zulip):

I think we should rework the way lints are setup anyway

Wesley Wiser (Sep 12 2018 at 16:28, on Zulip):

So, having thought it about some more, we shouldn't use optimized mir at all since that means the warning may or may not fire depending on optimization levels

nikomatsakis (Sep 12 2018 at 16:28, on Zulip):

right

nikomatsakis (Sep 12 2018 at 16:29, on Zulip):

I think what you would want to do is to have the MIR thief invoke the lint setup run

nikomatsakis (Sep 12 2018 at 16:29, on Zulip):

and not use any "lint runner" thing

nikomatsakis (Sep 12 2018 at 16:29, on Zulip):

probably we want to have some meta-query that is like "analyze"

nikomatsakis (Sep 12 2018 at 16:29, on Zulip):

alternatively, make MIR borrowck do it

nikomatsakis (Sep 12 2018 at 16:29, on Zulip):

same idea

nikomatsakis (Sep 12 2018 at 16:29, on Zulip):

(just that MIR borrowck is that meta-query)

Wesley Wiser (Sep 12 2018 at 16:29, on Zulip):

Hmm. So turn this lint into a query essentially?

nikomatsakis (Sep 12 2018 at 16:30, on Zulip):

yeah

nikomatsakis (Sep 12 2018 at 16:30, on Zulip):

it doesn't have to actually be a query

nikomatsakis (Sep 12 2018 at 16:30, on Zulip):

it can just be a fn that mir_borrowck calls

Wesley Wiser (Sep 12 2018 at 16:30, on Zulip):

Right

Wesley Wiser (Sep 12 2018 at 16:31, on Zulip):

But it also needs to run from the old borrow checker right?

nikomatsakis (Sep 12 2018 at 16:31, on Zulip):

no

nikomatsakis (Sep 12 2018 at 16:31, on Zulip):

I don't think so

nikomatsakis (Sep 12 2018 at 16:31, on Zulip):

I mean, we always invoke the MIR borrow check query

nikomatsakis (Sep 12 2018 at 16:31, on Zulip):

sometimes it does nothing

nikomatsakis (Sep 12 2018 at 16:31, on Zulip):

at least I think this is true

nikomatsakis (Sep 12 2018 at 16:31, on Zulip):

one second

Wesley Wiser (Sep 12 2018 at 16:32, on Zulip):

Even without -Zborrowck=mir?

nikomatsakis (Sep 12 2018 at 16:32, on Zulip):

right, we check that right here

nikomatsakis (Sep 12 2018 at 16:32, on Zulip):

so we could just invoke the lint before that :)

Wesley Wiser (Sep 12 2018 at 16:32, on Zulip):

Oh alright

nikomatsakis (Sep 12 2018 at 16:32, on Zulip):

one problem (maybe?) is that you'll get these lint messages before borrowck

Wesley Wiser (Sep 12 2018 at 16:32, on Zulip):

Makes sense

nikomatsakis (Sep 12 2018 at 16:32, on Zulip):

you can rejigger to run in whatever order...

Wesley Wiser (Sep 12 2018 at 16:33, on Zulip):

Are we going to want additional "mir lints" in the future?

nikomatsakis (Sep 12 2018 at 16:33, on Zulip):

probably?

nikomatsakis (Sep 12 2018 at 16:33, on Zulip):

I can think of a few ...

Wesley Wiser (Sep 12 2018 at 16:33, on Zulip):

Would it be worth adding a MirLint trait in addition to our EarlyLint and LateLint?

Wesley Wiser (Sep 12 2018 at 16:34, on Zulip):

I think there's an open issue about Copying large types... that's probably a good candidate for a mir lint too

nikomatsakis (Sep 12 2018 at 16:34, on Zulip):

that's the one I was thinking of. @qmx has a sort of working version of it, even

nikomatsakis (Sep 12 2018 at 16:35, on Zulip):

Would it be worth adding a MirLint trait in addition to our EarlyLint and LateLint?

I... maybe? I mean it's just a function that takes a MIR, basically?

nikomatsakis (Sep 12 2018 at 16:35, on Zulip):

I'm not a big fan of the current "register lint" system because it's shared mutable state

nikomatsakis (Sep 12 2018 at 16:35, on Zulip):

bad mojo

nikomatsakis (Sep 12 2018 at 16:35, on Zulip):

I think I'd rather we just invoke the lints by hand as we add them

Wesley Wiser (Sep 12 2018 at 16:35, on Zulip):

Yeah, I was thinking more about the registration infrastructure around the existing lints system

Wesley Wiser (Sep 12 2018 at 16:35, on Zulip):

Ah ok :)

nikomatsakis (Sep 12 2018 at 16:35, on Zulip):

I guess I don't have a strong opinion

nikomatsakis (Sep 12 2018 at 16:36, on Zulip):

but I think the lint infrastructure has to be changed to really fit the query model

qmx (Sep 12 2018 at 16:36, on Zulip):

I might have that branch laying around in my github

nikomatsakis (Sep 12 2018 at 16:36, on Zulip):

I think @eddyb had the idea of having people patch the query providers to insert themselves

nikomatsakis (Sep 12 2018 at 16:36, on Zulip):

so there'd be like a mir_lint query

nikomatsakis (Sep 12 2018 at 16:36, on Zulip):

by default, it does nothing

nikomatsakis (Sep 12 2018 at 16:37, on Zulip):

to add a mir_lint, you overwrite the provider with our own, and it invokes what was previously there somehow

nikomatsakis (Sep 12 2018 at 16:37, on Zulip):

not sure how that should work though

nikomatsakis (Sep 12 2018 at 16:37, on Zulip):

maybe it wants to be another kind of provider, a sort of "multi-provider"

nikomatsakis (Sep 12 2018 at 16:37, on Zulip):

(I think I prefer that idea)

Wesley Wiser (Sep 12 2018 at 16:39, on Zulip):

Could just be a helper query that demands lints for every item in the crate. Then we'd get caching from the query system and not rerun lints that don't need to be

Wesley Wiser (Sep 12 2018 at 16:39, on Zulip):

We'd have to cache the lint output though which I think we don't currently do

nikomatsakis (Sep 12 2018 at 16:40, on Zulip):

Could just be a helper query that demands lints for every item in the crate. Then we'd get caching from the query system and not rerun lints that don't need to be

the point is, how do you know what the set of lints to run is

nikomatsakis (Sep 12 2018 at 16:40, on Zulip):

if we just want a non-extensible set, seems fine, but we also need a way to support things like clippy

nikomatsakis (Sep 12 2018 at 16:40, on Zulip):

We'd have to cache the lint output though which I think we don't currently do

we do

nikomatsakis (Sep 12 2018 at 16:40, on Zulip):

at least, I belive we do

nikomatsakis (Sep 12 2018 at 16:40, on Zulip):

anyway, that is the basic idea, regardless

Wesley Wiser (Sep 12 2018 at 16:40, on Zulip):

Oh yes, I see

Wesley Wiser (Sep 12 2018 at 16:42, on Zulip):

I don't really know anything about Clippy...

Wesley Wiser (Sep 12 2018 at 16:42, on Zulip):

Regardless, it sounds like the right thing to do for now is to just call the function directly from mir_borrowck()

Wesley Wiser (Sep 12 2018 at 16:42, on Zulip):

That certainly resolves my issue.

simulacrum (Sep 12 2018 at 16:43, on Zulip):

to add a mir_lint, you overwrite the provider with our own, and it invokes what was previously there somehow

The way providers work today it's just a struct of function points so that would be fairly easy to do -- we'd just want the default to be an empty function for that provider instead of a function that panics (like today)

Wesley Wiser (Sep 12 2018 at 16:43, on Zulip):

Should I move the function out of librustc_lints then and into librustc_mir::borrow_checkor leave it where it is?

nikomatsakis (Sep 12 2018 at 16:44, on Zulip):

The way providers work today it's just a struct of function points so that would be fairly easy to do -- we'd just want the default to be an empty function for that provider instead of a function that panics (like today)

it's not that easy. You have to keep the old pointer.

nikomatsakis (Sep 12 2018 at 16:44, on Zulip):

this is why I would rather have a "multi-provider" opton

nikomatsakis (Sep 12 2018 at 16:44, on Zulip):

where the struct itself has a Vec or something

nikomatsakis (Sep 12 2018 at 16:44, on Zulip):

it also feels more declarative

Wesley Wiser (Sep 12 2018 at 16:44, on Zulip):

Is Clippy interested in running the built-in lints too?

nikomatsakis (Sep 12 2018 at 16:44, on Zulip):

(one question is whether we want to hash this set, too, we've not really .. figured out how that works with incremental)

Wesley Wiser (Sep 12 2018 at 16:46, on Zulip):

You could also have a query for additional_lints() and then mir_lint() just unions the built-in ones with anything returned from additional_lints(). rustc would return an empty set for additional_lints() but Clippy would return their lints.

nikomatsakis (Sep 12 2018 at 16:47, on Zulip):

what kind of value is that?

nikomatsakis (Sep 12 2018 at 16:47, on Zulip):

how do we hash it?

nikomatsakis (Sep 12 2018 at 16:48, on Zulip):

it sounds like you would be returning fn ptrs or trait objects

nikomatsakis (Sep 12 2018 at 16:48, on Zulip):

... we'd have to work that out

nikomatsakis (Sep 12 2018 at 16:48, on Zulip):

this is why I was pushing it into the providers, which is where fn poitners currently live

nikomatsakis (Sep 12 2018 at 16:48, on Zulip):

it doesn't exactly dodge the question

nikomatsakis (Sep 12 2018 at 16:48, on Zulip):

but it sort of does

nikomatsakis (Sep 12 2018 at 16:48, on Zulip):

because we can say that the "provider setup" routine

nikomatsakis (Sep 12 2018 at 16:49, on Zulip):

is kind of "hashed" somehow (e.g., with the names + versions of the plugins we run)

Wesley Wiser (Sep 12 2018 at 16:49, on Zulip):

You could always have a tuple of (metadata id, fn ptr) and just hash the metadata

nikomatsakis (Sep 12 2018 at 16:49, on Zulip):

yes, you could also do that

nikomatsakis (Sep 12 2018 at 16:49, on Zulip):

personally, I'd rather we not pass around random trait objects

Wesley Wiser (Sep 12 2018 at 16:49, on Zulip):

That's fair

nikomatsakis (Sep 12 2018 at 16:49, on Zulip):

(though I guess fn ptrs might be ok)

nikomatsakis (Sep 12 2018 at 16:49, on Zulip):

it just feels cleaner to push that into a setup phase

nikomatsakis (Sep 12 2018 at 16:50, on Zulip):

less room for error

Wesley Wiser (Sep 12 2018 at 16:50, on Zulip):

Regardless, I've got my answer for the UnconditionalRecursion lint question :)

nikomatsakis (Sep 12 2018 at 16:50, on Zulip):

:)

Wesley Wiser (Sep 12 2018 at 16:50, on Zulip):

Should I move that code out of librust_lints since it's not used there anymore or leave it since it is a lint?

nikomatsakis (Sep 12 2018 at 16:51, on Zulip):

I think you should move it out, maybe make librustc_mir/lints?

Wesley Wiser (Sep 12 2018 at 16:51, on Zulip):

Not super important I suppose but I don't know what the desired architecture is here

nikomatsakis (Sep 12 2018 at 16:51, on Zulip):

yeah, who knows :)

Wesley Wiser (Sep 12 2018 at 16:52, on Zulip):

Ok, will do!

nikomatsakis (Sep 12 2018 at 16:52, on Zulip):

I'm opinionated but others may also be

nikomatsakis (Sep 12 2018 at 16:52, on Zulip):

but I'd rather not make librustc_mir depend librustc_lints

nikomatsakis (Sep 12 2018 at 16:52, on Zulip):

so .. :)

Wesley Wiser (Sep 12 2018 at 16:52, on Zulip):

Good point

Wesley Wiser (Sep 12 2018 at 16:53, on Zulip):

Thanks @nikomatsakis!

simulacrum (Sep 12 2018 at 19:26, on Zulip):

@nikomatsakis Hm, yeah, but in theory you're only going to register providers once so you could stash it in a static oncecell (not great though)

nikomatsakis (Sep 12 2018 at 19:39, on Zulip):

seems messier than supporting multi-providers or something

simulacrum (Sep 12 2018 at 19:39, on Zulip):

Sure, yeah, I agree -- was just saying that if we want to avoid overhauling that (it feels like a somewhat non trivial change) there is a way

nikomatsakis (Sep 12 2018 at 19:40, on Zulip):

yes, ok. I actually think it'd be a fairly small change but I may be overlooking stuff

nikomatsakis (Sep 12 2018 at 19:40, on Zulip):

I bet @eddyb has a better idea anyway :)

nikomatsakis (Sep 12 2018 at 19:40, on Zulip):

they're usually 2 steps ahead on this sort of thing

simulacrum (Sep 12 2018 at 19:42, on Zulip):

well the query stuff is macros all the way down last I looked which makes me think individual changes could be quite painful

nikomatsakis (Sep 12 2018 at 19:42, on Zulip):

yeah, it is macros all the way down...

nikomatsakis (Sep 12 2018 at 19:43, on Zulip):

/me shrugs

Wesley Wiser (Sep 12 2018 at 19:43, on Zulip):

/me shudders

nikomatsakis (Sep 12 2018 at 19:43, on Zulip):

I've been wondering about trying to factor it out into a crate. It's hard to do

simulacrum (Sep 12 2018 at 19:44, on Zulip):

I think the main blocker there is that it is used by rustc itself and uses types defined in it (e.g., DefId)

simulacrum (Sep 12 2018 at 19:45, on Zulip):

I think moving the HIR declarations into a crate that librustc depends on might help but not sure

simulacrum (Sep 12 2018 at 19:45, on Zulip):

Generally I think the query providers could use a refactor to be less downwardly structured but not sure

nikomatsakis (Sep 12 2018 at 19:47, on Zulip):

sorry, I meant something more drastic, like a crates.io crate :)

nikomatsakis (Sep 12 2018 at 19:47, on Zulip):

but it is related; it'd be nice, for example, to have the set of queries be dynamically extensible

simulacrum (Sep 12 2018 at 19:47, on Zulip):

Well, yeah - I meant that even decoupling inside the compiler is hard today

simulacrum (Sep 12 2018 at 19:47, on Zulip):

Much less moving out to crates.io :)

nikomatsakis (Sep 12 2018 at 19:49, on Zulip):

:)

eddyb (Sep 12 2018 at 20:31, on Zulip):

uhhh this is a long discussion

eddyb (Sep 12 2018 at 20:31, on Zulip):

please do not complicate providers

eddyb (Sep 12 2018 at 20:33, on Zulip):

IMO hardwired lints should be implemented in the place where they have all the information they need. this is partly why we moved to the "lint scopes" approach where you don't need the messy "lint runner" at all

eddyb (Sep 12 2018 at 20:33, on Zulip):

so we don't really need any queries for these

eddyb (Sep 12 2018 at 20:33, on Zulip):

just move the code to rustc_mir

eddyb (Sep 12 2018 at 20:34, on Zulip):

making them hookable is interesting but I had a different idea than @nikomatsakis seems to have discussed - or maybe I missed a part of the discussion

eddyb (Sep 12 2018 at 20:35, on Zulip):

that is, I imagined one of the things we could do is read-only hooks for queries. but this is not enough for unconditional recursion (really excited that's moving to MIR :D), unless we get rid of stealing

eddyb (Sep 12 2018 at 20:36, on Zulip):

or, wait, is unconditional recursion a local analysis or not?

eddyb (Sep 12 2018 at 20:36, on Zulip):

if it's local then it doesn't need anything special

eddyb (Sep 12 2018 at 20:37, on Zulip):

anyway we have to be really careful with the design of any sort of additional hooks, so they don't introduce performance or architectural overhead

eddyb (Sep 12 2018 at 20:38, on Zulip):

(e.g. incremental interactions wrt diagnostics produced by hooks)

eddyb (Sep 12 2018 at 20:40, on Zulip):

I want open-world queries but it's an even harder problem. we'll eventually need something for incremental parsing and macro expansion

eddyb (Sep 12 2018 at 20:41, on Zulip):

it's almost an ECS problem, where open-world implementations typically add some overhead

eddyb (Sep 12 2018 at 20:41, on Zulip):

what we need is similar to dynamic linking support for TLS

Wesley Wiser (Sep 12 2018 at 20:42, on Zulip):

@eddyb By local do you mean "only looks at the function body"?

eddyb (Sep 12 2018 at 20:42, on Zulip):

where each crate can add their own queries and they're "concatenated" in the engine, with statics adjusted to indicate offsets

eddyb (Sep 12 2018 at 20:43, on Zulip):

@Wesley Wiser yeah

Wesley Wiser (Sep 12 2018 at 20:43, on Zulip):

Yeah, it's local

eddyb (Sep 12 2018 at 20:43, on Zulip):

then we should be able to run it post-MIR-building

eddyb (Sep 12 2018 at 20:43, on Zulip):

or somewhere in there

eddyb (Sep 12 2018 at 20:44, on Zulip):

all it needs is "call dominates return" and Instance, really

eddyb (Sep 12 2018 at 20:47, on Zulip):

so, domtree after building MIR, should be a tiny lint

eddyb (Sep 12 2018 at 20:48, on Zulip):

(sorry, I was on my phone, and I'm a bit sleepy)

eddyb (Sep 12 2018 at 20:49, on Zulip):

I want a redesign of rustc but I'm worried that if we don't organize properly we'll be left with a bunch of suboptimal ideas, and even worse, implement some of them

nikomatsakis (Sep 12 2018 at 20:56, on Zulip):

just move the code to rustc_mir

to be clear @eddyb, this is precisely what I advised @Wesley Wiser to do...

nikomatsakis (Sep 12 2018 at 20:56, on Zulip):

the rest was just random, non-actionable chatter

nikomatsakis (Sep 12 2018 at 20:57, on Zulip):

no need to panic :)

eddyb (Sep 12 2018 at 20:57, on Zulip):

heh

Wesley Wiser (Sep 12 2018 at 20:57, on Zulip):

and that's my current plan :)

eddyb (Sep 12 2018 at 20:57, on Zulip):

since I got pinged and the conversation was long, I wanted to state my position

nikomatsakis (Sep 12 2018 at 20:57, on Zulip):

makes sense

nikomatsakis (Sep 12 2018 at 20:58, on Zulip):

I figured you'd have a plan, I'm not sure I fully understood it, but it's not imp't right now :)

eddyb (Sep 12 2018 at 20:59, on Zulip):

I never have a plan

eddyb (Sep 12 2018 at 20:59, on Zulip):

only a loose set of ideas

eddyb (Sep 12 2018 at 21:00, on Zulip):

like https://github.com/rust-lang/rust/issues/53590

Wesley Wiser (Sep 12 2018 at 21:04, on Zulip):

@eddyb That looks really interesting!

eddyb (Sep 12 2018 at 21:04, on Zulip):

the hard part is having enough time to experiment with all of the ideas

Wesley Wiser (Sep 12 2018 at 21:05, on Zulip):

Yeah

Wesley Wiser (Sep 12 2018 at 21:06, on Zulip):

Completely off topic: Is there a list of "desired" MIR optimization passes that have yet to be implemented?

Wesley Wiser (Sep 12 2018 at 21:06, on Zulip):

I've seen comments about such things but nothing concrete

Wesley Wiser (Sep 12 2018 at 21:06, on Zulip):

And it sounds like something I'd love to work on :)

eddyb (Sep 12 2018 at 21:06, on Zulip):

I have a few unfinished passes

eddyb (Sep 12 2018 at 21:07, on Zulip):

partly blocked on an unfinished branch to redo debuginfo wrt MIR

eddyb (Sep 12 2018 at 21:07, on Zulip):

I almost forgot about all of that stuff shudder

eddyb (Sep 12 2018 at 21:09, on Zulip):

but there's lint(-like) stuff that we could probably move to the MIR

eddyb (Sep 12 2018 at 21:09, on Zulip):

like some unreachable code warnings might belong there instead of typeck?

eddyb (Sep 12 2018 at 21:09, on Zulip):

OH OH

eddyb (Sep 12 2018 at 21:09, on Zulip):

@Wesley Wiser liveness

eddyb (Sep 12 2018 at 21:09, on Zulip):

(unless the MIR borrowck already warns about unused assignments and stuff)

Wesley Wiser (Sep 12 2018 at 21:10, on Zulip):

@eddyb Is that https://github.com/rust-lang/rust/issues/51003 ?

eddyb (Sep 12 2018 at 21:10, on Zulip):

yupp

Wesley Wiser (Sep 12 2018 at 21:10, on Zulip):

Interesting...

eddyb (Sep 12 2018 at 21:10, on Zulip):

I'm not sure how good the rustc_mir::util::liveness thing is

eddyb (Sep 12 2018 at 21:11, on Zulip):

like, it's potentially too specific to the generator state transform to be generally usable

eddyb (Sep 12 2018 at 21:11, on Zulip):

but if it is usable, then it should make the analysis pretty straight-forward

Wesley Wiser (Sep 12 2018 at 21:11, on Zulip):

Gotcha

eddyb (Sep 12 2018 at 21:11, on Zulip):

compared to the huge mess of HIR-matching code that the old liveness pass is

nikomatsakis (Sep 12 2018 at 21:12, on Zulip):

it's reasonable general, but it doesn't do some of the stuff the old analysis did

Wesley Wiser (Sep 12 2018 at 21:12, on Zulip):

I'll take a look at that after I finish porting UnconditionalRecursion

nikomatsakis (Sep 12 2018 at 21:12, on Zulip):

e.g., unused assignment

nikomatsakis (Sep 12 2018 at 21:12, on Zulip):

well, I forget, maybe you can get that particular thing

nikomatsakis (Sep 12 2018 at 21:12, on Zulip):

but I remember there were some extra variations on liveness

nikomatsakis (Sep 12 2018 at 21:13, on Zulip):

the dataflow stuff may or may not be relevant

nikomatsakis (Sep 12 2018 at 21:13, on Zulip):

it only does fwd dataflow

Wesley Wiser (Sep 13 2018 at 15:21, on Zulip):

Ok, so this is going pretty well. I moved the code around and I've got it being called from the MIR borrow check entry point as discussed. That's all working but the src/test/ui/lints/lint-unconditional-recursion.rs test is failing in a few cases. It looks like the issue is around calling trait methods. For example:

trait Foo {
    fn bar(&self) {
        self.bar()
    }
}

impl Foo for i32 {
    fn bar(&self) { //~ ERROR function cannot return without recursing
        0.bar()
    }
}

Isn't triggering the error.

Wesley Wiser (Sep 13 2018 at 15:22, on Zulip):

Right now, the lint is just checking for TerminatorKind::Call(fn) where fn's DefId == our function's DefId. Which obviously doesn't work in this case because the function we're calling is Foo::bar but our function is the impl's function

Wesley Wiser (Sep 13 2018 at 15:23, on Zulip):

I think I need to actually calculate the real method we're calling on the trait. Which, I think, is what the rustc::ty::instance::Instance::resolve() function does, right?

Wesley Wiser (Sep 13 2018 at 15:23, on Zulip):

https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/instance/struct.Instance.html#method.resolve

eddyb (Sep 13 2018 at 17:49, on Zulip):

yeah! doesn't the old code already use Instance? or is it resolving things manually?

Wesley Wiser (Sep 13 2018 at 17:50, on Zulip):

No, it re-invents the wheel I think

Wesley Wiser (Sep 13 2018 at 17:51, on Zulip):

/me looking

Wesley Wiser (Sep 13 2018 at 17:51, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_lint/builtin.rs#L998

eddyb (Sep 13 2018 at 17:52, on Zulip):

traits::SelectionContext::new oh jeez indeed it does

Wesley Wiser (Sep 13 2018 at 17:54, on Zulip):

Should I use that instead of Instance?

Wesley Wiser (Sep 13 2018 at 17:54, on Zulip):

(not quite sure what I'm looking at)

eddyb (Sep 13 2018 at 17:55, on Zulip):

you want Instance

Wesley Wiser (Sep 13 2018 at 17:56, on Zulip):

Ok :)

eddyb (Sep 13 2018 at 17:56, on Zulip):

(btw, feel free to ping me about compiler minutia - oh and discord works better, notification-wise, I think)

Wesley Wiser (Sep 13 2018 at 17:57, on Zulip):

I also looked at tcx.subst_and_normalize_erasing_regions() but I couldn't really figure out what that did :laughing:

Wesley Wiser (Sep 13 2018 at 17:57, on Zulip):

Absolutely. Thanks @eddyb!

nikomatsakis (Sep 13 2018 at 18:01, on Zulip):

@Wesley Wiser (are you rewriting it to use Instance?)

nikomatsakis (Sep 13 2018 at 18:02, on Zulip):

I forgot about these messages, sorry, meant to follow up with you

Wesley Wiser (Sep 13 2018 at 18:02, on Zulip):

Yeah, that's what I was going to try to do

nikomatsakis (Sep 13 2018 at 18:02, on Zulip):

I can also explain what subst_and_normalize_erasing_regions does if you want :)

nikomatsakis (Sep 13 2018 at 18:02, on Zulip):

ok

Wesley Wiser (Sep 13 2018 at 18:02, on Zulip):

I was trying to avoid porting those helper functions

Wesley Wiser (Sep 13 2018 at 18:03, on Zulip):

In particular, I wasn't sure how to get a NodeId for the call expression from the Mir

Wesley Wiser (Sep 13 2018 at 18:04, on Zulip):

I can also explain what subst_and_normalize_erasing_regions does if you want :)

Absolutely! I'm kind of just curious

nikomatsakis (Sep 13 2018 at 18:07, on Zulip):

In particular, I wasn't sure how to get a NodeId for the call expression from the Mir

why do you need that?

nikomatsakis (Sep 13 2018 at 18:07, on Zulip):

Absolutely! I'm kind of just curious

well the "subst" part means replace the generic types with their values

nikomatsakis (Sep 13 2018 at 18:07, on Zulip):

the "normalize" part means to look for associated types that may now be resolveable

nikomatsakis (Sep 13 2018 at 18:07, on Zulip):

e.g., if you had a generic type like Vec<<T as Iterator>::Item>

nikomatsakis (Sep 13 2018 at 18:08, on Zulip):

and you replace T with vec::Iter<'x, u32> or something

nikomatsakis (Sep 13 2018 at 18:08, on Zulip):

then you can normalize Vec<<vec::Iter<'x, u32> as Iterator>::Item> to Vec<&'x u32>

nikomatsakis (Sep 13 2018 at 18:08, on Zulip):

the "erasing regions" part takes that result and replaces all the (free) regions with a special 'erased; we do this a lot when generating code or in some phases of the compiler where we don't care about the regions

nikomatsakis (Sep 13 2018 at 18:08, on Zulip):

so you get Vec<&'erased u32>

nikomatsakis (Sep 13 2018 at 18:08, on Zulip):

or just Vec<&u32>, if you prefer

nikomatsakis (Sep 13 2018 at 18:09, on Zulip):

(note: "region" is a synonym with "lifetime")

Wesley Wiser (Sep 13 2018 at 18:09, on Zulip):

The helper functions in the lint right now are all expr based and they need the NodeId of the call expression. For example: https://github.com/rust-lang/rust/blob/994cdd918589535d705177545bf503cd0c3c5148/src/librustc_lint/builtin.rs#L1011

nikomatsakis (Sep 13 2018 at 18:10, on Zulip):

oh, in the lint, I see

nikomatsakis (Sep 13 2018 at 18:10, on Zulip):

yeah ugh

nikomatsakis (Sep 13 2018 at 18:10, on Zulip):

:)

nikomatsakis (Sep 13 2018 at 18:11, on Zulip):

these should be expressable in MIR but I guess they have to be rewritten from scratch

Wesley Wiser (Sep 13 2018 at 18:11, on Zulip):

the "erasing regions" part takes that result and replaces all the (free) regions with a special 'erased; we do this a lot when generating code or in some phases of the compiler where we don't care about the regions

And this is because after we've done borrow checking, we know the lifetimes are valid and since you can't specialize on lifetimes, they have no effect to the rest of the compiler?

Wesley Wiser (Sep 13 2018 at 18:12, on Zulip):

these should be expressable in MIR but I guess they have to be rewritten from scratch

Yeah. In my original version that used optimized_mir() this seemed to "just work". Ie, this test was passing. I guess optimized mir has already selected the correct functions for trait methods calls?

Wesley Wiser (Sep 13 2018 at 18:14, on Zulip):

subst_and_normalize_erasing_regions()

So I guess my confusion was the T parameter. I see there's a bound on that to make it work but I just wasn't sure what to pass there. I've got a DefId for the trait method I'm calling. I've got an Operand for the call. Also a Const that wraps the function. I think all of those could be passed but I wasn't sure that what I'd get back is what I want

Wesley Wiser (Sep 13 2018 at 18:15, on Zulip):

Is there a difference between Instance::resolve() and subst_and_normalize_erasing_regions<DefId>()?

nikomatsakis (Sep 13 2018 at 18:18, on Zulip):

Yeah. In my original version that used optimized_mir() this seemed to "just work". Ie, this test was passing. I guess optimized mir has already selected the correct functions for trait methods calls?

I don't really understand this at all

nikomatsakis (Sep 13 2018 at 18:19, on Zulip):

there is no difference between optimized-mir and non-optimized MIR in this respect

Wesley Wiser (Sep 13 2018 at 18:19, on Zulip):

Hmm....

nikomatsakis (Sep 13 2018 at 18:19, on Zulip):

Is there a difference between Instance::resolve() and subst_and_normalize_erasing_regions<DefId>()?

let me go read the source; in my head they are not particularly related to one another :)

Wesley Wiser (Sep 13 2018 at 18:20, on Zulip):

That was a few days ago. Perhaps I'm just misremembering which tests were broken?

Wesley Wiser (Sep 13 2018 at 18:20, on Zulip):

It's not really important

qmx (Sep 13 2018 at 18:20, on Zulip):

/me vaguely remembers fiddling with Instance::resolve()

nikomatsakis (Sep 13 2018 at 18:21, on Zulip):

I guess they are related, in that subst_and_normalize_erasing_regions is sort of maybe a building block for Instance::resolve

nikomatsakis (Sep 13 2018 at 18:21, on Zulip):

in any case, the MIR just records the trait + types

nikomatsakis (Sep 13 2018 at 18:21, on Zulip):

(when you are invoking a trait method)

Wesley Wiser (Sep 13 2018 at 18:21, on Zulip):

Is there a difference between Instance::resolve() and subst_and_normalize_erasing_regions<DefId>()?

let me go read the source; in my head they are not particularly related to one another :)

They only seemed related to me in that they have, what I would expect, is the right function signature for what I want. They both take a DefId, ParamEnv, and Substs which is what I was thinking you'd need to resolve the trait method call

nikomatsakis (Sep 13 2018 at 18:21, on Zulip):

the job of Instance::resolve is to try to match that up with a specific impl or other thing

eddyb (Sep 13 2018 at 18:22, on Zulip):

@Wesley Wiser what do you have already? the lint helpers are useless because they're for the AST. what I think you need is to look at TerminatorKind::Call and the type of the function to call from it, if it's ty::FnDef(def_id, substs) you can feed those def_id and substs to Instance::resolve

nikomatsakis (Sep 13 2018 at 18:22, on Zulip):

ah well subst_and_normalize_erasing_regions would not normally be invoked on a DefId

nikomatsakis (Sep 13 2018 at 18:22, on Zulip):

that is a no-op

nikomatsakis (Sep 13 2018 at 18:22, on Zulip):

it would be invoked on someting with types in it, usually

nikomatsakis (Sep 13 2018 at 18:22, on Zulip):

looking at the inline code may be helpful

eddyb (Sep 13 2018 at 18:22, on Zulip):

heh I should've suggested inline

nikomatsakis (Sep 13 2018 at 18:23, on Zulip):

e.g., this part

Wesley Wiser (Sep 13 2018 at 18:23, on Zulip):

@eddyb Yep, that's what I've got. I should have pushed a branch last night to my repo so I could just link it

I've got a DefId for the function being called, its Substs and I can get access to the ParamEnv via the tcx.param_env() query

eddyb (Sep 13 2018 at 18:23, on Zulip):

note that the param_env is that of the caller, not callee

eddyb (Sep 13 2018 at 18:23, on Zulip):

@nikomatsakis oh that's wrong

eddyb (Sep 13 2018 at 18:23, on Zulip):

/me opens issue

Wesley Wiser (Sep 13 2018 at 18:23, on Zulip):

I want the param_env(fn_being_linted) right?

eddyb (Sep 13 2018 at 18:24, on Zulip):

yeah

Wesley Wiser (Sep 13 2018 at 18:24, on Zulip):

Ok, cool

eddyb (Sep 13 2018 at 18:24, on Zulip):

you'd query it only once and use it later

Wesley Wiser (Sep 13 2018 at 18:24, on Zulip):

Gotcha

Wesley Wiser (Sep 13 2018 at 18:25, on Zulip):

@nikomatsakis Oh, that's helpful. Thanks! I thought maybe there was code that did what I wanted in the codegen layer but I couldn't find it

eddyb (Sep 13 2018 at 18:25, on Zulip):

it's in rustc_codegen_llvm/mir/block.rs

eddyb (Sep 13 2018 at 18:25, on Zulip):

but it's uglier

eddyb (Sep 13 2018 at 18:26, on Zulip):

https://github.com/rust-lang/rust/issues/54193

eddyb (Sep 13 2018 at 18:27, on Zulip):

make sure you don't make the same mistake the inline code does

nikomatsakis (Sep 13 2018 at 18:28, on Zulip):

@eddyb wait, what's wrong exactly?

nikomatsakis (Sep 13 2018 at 18:28, on Zulip):

/me looks at the inline code again

nikomatsakis (Sep 13 2018 at 18:29, on Zulip):

I remember there was some ICE, maybe it's related?

Wesley Wiser (Sep 13 2018 at 18:29, on Zulip):

@eddyb I have a pattern match that looks just like that right now. I should just call Operand::ty(mir, tcx) instead?

nikomatsakis (Sep 13 2018 at 18:31, on Zulip):

I don't see what's wrong about the inline code yet :)

eddyb (Sep 13 2018 at 18:43, on Zulip):

yeah you should call the method

eddyb (Sep 13 2018 at 18:43, on Zulip):

and change the inline code while you're at it

eddyb (Sep 13 2018 at 18:44, on Zulip):

@nikomatsakis I linked an issue

eddyb (Sep 13 2018 at 18:44, on Zulip):

or are you saying the issue isn't clear enough?

nikomatsakis (Sep 13 2018 at 18:44, on Zulip):

oh

nikomatsakis (Sep 13 2018 at 18:44, on Zulip):

I didn't see that

nikomatsakis (Sep 13 2018 at 18:44, on Zulip):

I see it now

nikomatsakis (Sep 13 2018 at 18:44, on Zulip):

so you're saying it is insufficiently general

nikomatsakis (Sep 13 2018 at 18:44, on Zulip):

but not that it's using the wrong param_env or something

nikomatsakis (Sep 13 2018 at 18:44, on Zulip):

I thought you meant the latter

eddyb (Sep 13 2018 at 18:46, on Zulip):

right, it's only a mistake in the sense of missing the more general approach, not incorrectness

Wesley Wiser (Sep 13 2018 at 18:51, on Zulip):

@eddyb Got it :thumbs_up:

Last update: Nov 22 2019 at 04:30UTC