Stream: t-compiler

Topic: #53821 Report const eval error inside the query


pnkfelix (Sep 20 2018 at 15:22, on Zulip):

lets continue talking here (continuation of this conversation)

pnkfelix (Sep 20 2018 at 15:22, on Zulip):

cc @T-compiler and @RalfJ

nagisa (Sep 20 2018 at 15:23, on Zulip):

So my questions are:

1. What code, besides macros breaks here;
2. I suppose we don’t want to change the strategy employed by this PR much (i.e. our ultimate goal is to error everywhere anyway)?

nagisa (Sep 20 2018 at 15:24, on Zulip):

3. How does this interact with constevaluated code that is really a runtime code?

nagisa (Sep 20 2018 at 15:24, on Zulip):

that is, there was an issue before about println!("{}", 1/0) not failing if put in a main (for good reasons IIRC). Would this PR landing change this behaviour in any way?

RalfJ (Sep 20 2018 at 15:25, on Zulip):

in the current state the PR shouldnt introduce any new errors

RalfJ (Sep 20 2018 at 15:26, on Zulip):

but it also does not lint any more when an unused type contains a constant that errors when executed

nagisa (Sep 20 2018 at 15:26, on Zulip):

unused type, as in type A = [42; BAD_CONST]?

nagisa (Sep 20 2018 at 15:26, on Zulip):

where A is not used?

RalfJ (Sep 20 2018 at 15:27, on Zulip):

previously, both the lint and WF (or whatever evaluates constants when a type gets actually used) used the same query, but the query just returned "well there was an error" and it was up to the caller to figure out what to do with that error

RalfJ (Sep 20 2018 at 15:27, on Zulip):

so the lint made it a warning, WF made it a hard error

RalfJ (Sep 20 2018 at 15:27, on Zulip):

now the query already reports the error, so we cannot make that distinction any more

RalfJ (Sep 20 2018 at 15:27, on Zulip):

we also stop reporting the same error 3 times because nobody was sure if some other part didnt already report the error...

eddyb (Sep 20 2018 at 15:27, on Zulip):

okay but I have an upcoming PR that could make it an unconditional error

RalfJ (Sep 20 2018 at 15:27, on Zulip):

@nagisa yes

eddyb (Sep 20 2018 at 15:28, on Zulip):

because it's more uniform IMO to do so (if we can get away with it)

RalfJ (Sep 20 2018 at 15:28, on Zulip):

@eddyb sure fine for me, but that doesnt block this PR, right?

nagisa (Sep 20 2018 at 15:28, on Zulip):

@RalfJ ah, now I understand what’s the point of discussion finally :P

eddyb (Sep 20 2018 at 15:28, on Zulip):

@RalfJ well, if we don't make an error, we can't land the PR

RalfJ (Sep 20 2018 at 15:29, on Zulip):

@nagisa as for your point 3, it shouldnt. that would be const_prop, which opportunistically evaluates runtime code with the miri engine. it's a horrible mess, but it also doesnt use the const_eval query and hence shouldnt be affected by this change

eddyb (Sep 20 2018 at 15:29, on Zulip):

because we can't tell the query to warn instead anymore

RalfJ (Sep 20 2018 at 15:29, on Zulip):

@eddyb why not?

RalfJ (Sep 20 2018 at 15:29, on Zulip):

so?

RalfJ (Sep 20 2018 at 15:29, on Zulip):

what warning is fairly recent

RalfJ (Sep 20 2018 at 15:29, on Zulip):

seems better to me to lose it again than the mess we had previously

eddyb (Sep 20 2018 at 15:29, on Zulip):

I mean, we're moving towards a self-consistent model of type aliases

RalfJ (Sep 20 2018 at 15:29, on Zulip):

and then figure out something better

RalfJ (Sep 20 2018 at 15:29, on Zulip):

@eddyb the query never warned for unused types

RalfJ (Sep 20 2018 at 15:30, on Zulip):

instead we have a really strange lint, which all it does is walk the crate and fire all const_eval queries

eddyb (Sep 20 2018 at 15:30, on Zulip):

so if we warn about type Foo = Vec<str>;, we should warn about type Foo = [u8; 0 - 1]; too

RalfJ (Sep 20 2018 at 15:31, on Zulip):

so if we warn about type Foo = Vec<str>;, we should warn about type Foo = [u8; 0 - 1]; too

sure but that would likely fall out of whatever it is you are doing, and not out of the UnusedErrConstant lint or whatever oli called it

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

yes but what I'd be doing would necessarily query const_eval

RalfJ (Sep 20 2018 at 15:31, on Zulip):

yes

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

and I can't force it now to warn, it would error

RalfJ (Sep 20 2018 at 15:31, on Zulip):

yes

nagisa (Sep 20 2018 at 15:31, on Zulip):

#MakeConstEvalReturnResult<T, E>

eddyb (Sep 20 2018 at 15:32, on Zulip):

@nagisa it does, before the PR

RalfJ (Sep 20 2018 at 15:32, on Zulip):

@nagisa thats what we have currently, it is horrible

RalfJ (Sep 20 2018 at 15:32, on Zulip):

so the only regression in the const query error PR is that it stops warning for bad consts in unused types

eddyb (Sep 20 2018 at 15:32, on Zulip):

it requires reifying a ridiculous amount of state in the error

RalfJ (Sep 20 2018 at 15:32, on Zulip):

which IIRC isnt even on stable yet

nagisa (Sep 20 2018 at 15:32, on Zulip):

yeah, I realised after I sent it. Failing in const_eval directly, however, precludes using const_eval for other uses.

RalfJ (Sep 20 2018 at 15:33, on Zulip):

for now, that seems like an acceptable regression to me for the overall cleanup

nagisa (Sep 20 2018 at 15:33, on Zulip):

can we just add two distinct variants of const eval query, one that reports the errors from within itself, and the other that returns Option/Result?

RalfJ (Sep 20 2018 at 15:34, on Zulip):

that would be 4, we already have const_eval and const_eval_raw

nagisa (Sep 20 2018 at 15:34, on Zulip):

Similar to the const_eval_raw/const_eval split, yeah.

RalfJ (Sep 20 2018 at 15:34, on Zulip):

where const_eval does the actual work of computing a const

RalfJ (Sep 20 2018 at 15:34, on Zulip):

and const_eval_raw tests the value at the given type

RalfJ (Sep 20 2018 at 15:34, on Zulip):

I dont know why that cant happen in one query

RalfJ (Sep 20 2018 at 15:34, on Zulip):

@Oli said sth with cycles

nagisa (Sep 20 2018 at 15:35, on Zulip):

making a "test" query would work for eddyb’s use-cases I think?

RalfJ (Sep 20 2018 at 15:35, on Zulip):

it could also be an argument to the query

nagisa (Sep 20 2018 at 15:35, on Zulip):

like, the type stuff should not depend on an actual const value at all, just whether it has errors or not.

RalfJ (Sep 20 2018 at 15:35, on Zulip):

but then we have to be careful about caching

RalfJ (Sep 20 2018 at 15:36, on Zulip):

well I guess its isomorphic to two queries

RalfJ (Sep 20 2018 at 15:36, on Zulip):

so we have to be careful either way^^

nagisa (Sep 20 2018 at 15:36, on Zulip):

All that being said, I think I’d be fine with a temporary regression here.

nagisa (Sep 20 2018 at 15:37, on Zulip):

/me shrugs

nagisa (Sep 20 2018 at 15:37, on Zulip):

/me has a feeling that one way or the other a query split between "compute" and "test" will one way or the other get exposed to other parts of the compiler.

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

ok so I read this stuff :)

nikomatsakis (Sep 20 2018 at 18:14, on Zulip):

@eddyb @RalfJ is one of you evaluating whether we think we can get away with making this a hard error all the time?

It feels like "borderline bug territory" indeed.

That said, I don't know why we couldn't have an "inner query" that returns a Result and another query that makes it an error (which invokes the "inner query"). Most users would use the outer query.

nikomatsakis (Sep 20 2018 at 18:14, on Zulip):

I guess this is what @nagisa was proposing?

eddyb (Sep 20 2018 at 18:15, on Zulip):

@nikomatsakis my latest PR will be used to crater for type aliases, not just constants in them, but other monomorphic non-WF things

nikomatsakis (Sep 20 2018 at 18:15, on Zulip):

that PR currently makes things a warning, right?

nikomatsakis (Sep 20 2018 at 18:16, on Zulip):

(PS I'm having a hard time keeping "all the things" sorted it out here when it comes to your many PRs, but I'm sort of assuming you'll kick me when you feel like you've worked it out to a point where you need a review, and/or have a burning question?)

nikomatsakis (Sep 20 2018 at 18:16, on Zulip):

which reminds me that we should talk about errors in 2018 and how aggressive we think we can be

eddyb (Sep 20 2018 at 18:16, on Zulip):

it makes type Foo<T> = NeedsBounds<T>; a warning but type Bar = Vec<str>; (or type Baz = [u8; 0 - 1];) a hard error

nikomatsakis (Sep 20 2018 at 18:16, on Zulip):

but not probably in this topic :)

RalfJ (Sep 20 2018 at 18:16, on Zulip):

having the miri engine error type in a query is a bit messy, but that's mostly me not wanting to update 15 manually derived traits each time that changes :P

nikomatsakis (Sep 20 2018 at 18:17, on Zulip):

it makes type Foo<T> = NeedsBounds<T>; a warning but type Bar = Vec<str> a hard error

what is the distinction between those? is it based on the use of a type parameter?

nikomatsakis (Sep 20 2018 at 18:17, on Zulip):

we do something similar for defaults

nikomatsakis (Sep 20 2018 at 18:17, on Zulip):

e.g., struct Foo<T = Vec<str>> is a hard error

eddyb (Sep 20 2018 at 18:17, on Zulip):

yes, that's the entire point

nikomatsakis (Sep 20 2018 at 18:17, on Zulip):

ok, I see

nikomatsakis (Sep 20 2018 at 18:17, on Zulip):

I .. think we do that for defaults, anyway?

nikomatsakis (Sep 20 2018 at 18:17, on Zulip):

we went back and forth and wound up settling on something pretty conservative iirc

nikomatsakis (Sep 20 2018 at 18:17, on Zulip):

having the miri engine error type in a query is a bit messy, but that's mostly me not wanting to update 15 manually derived traits each time that changes :P

I see, I see

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

so, there are two things:

oli (Sep 26 2018 at 12:31, on Zulip):

We don't want to pass state to the query, as that would duplicate errors and work (the latter can be worked around by having more queries as noted above). The entire point of the PR was to deduplicate errors and centralize error reporting in one location.

oli (Sep 26 2018 at 12:34, on Zulip):

We could add a query which returns a Diagnostic object in the err case, which is then reported by the main query. Type alias array lengths could then use the inner query to report the error as a future incompat lint

Last update: Nov 16 2019 at 01:05UTC