Stream: t-compiler

Topic: #62307 `#[structural_match]` only checked in shallow manner


pnkfelix (Jul 04 2019 at 11:41, on Zulip):

Hey @nikomatsakis

pnkfelix (Jul 04 2019 at 11:41, on Zulip):

can you take a moment to convince me that putting support for a "missing #[structural_match]" flag into TypeFlags is a bad idea?

pnkfelix (Jul 04 2019 at 11:42, on Zulip):

or rather, I cannot immediately even tell if the computation of TypeFlags has any access to the tcx, and thus I do not know if it can do lookups for the attributes on AdtDef's it find.

pnkfelix (Jul 04 2019 at 11:43, on Zulip):

So its possible that would be a reason that putting a bit there would be a bad idea. But in any case, I'm basically floundering around trying to figure out the "right way" to resolve this one remaining case I have identifed, which is given in this comment

pnkfelix (Jul 04 2019 at 12:13, on Zulip):

The simpler short term approach is to do a recursion over the fields of the ADT locally (with an FxHashSet to track previously traversed types) ... this is probably an anti-pattern but its one I see used elsewhere. I wonder if the rustc-guide has anything to say about such code.

pnkfelix (Jul 09 2019 at 14:58, on Zulip):

Hey again @nikomatsakis ; so you are now well aware of PR #62339 (which took a different route to solving this problem).

pnkfelix (Jul 09 2019 at 14:58, on Zulip):

But I have two questions: You said a crater run would be a good idea. Did you want that to block this PR? You said in the previous bullet "r=me" on PR itself, so I take that to mean that the crater run is intended to be a follow-up task, right?

nikomatsakis (Jul 09 2019 at 15:01, on Zulip):

Correct

nikomatsakis (Jul 09 2019 at 15:01, on Zulip):

Follow-up task seems fine

nikomatsakis (Jul 09 2019 at 15:01, on Zulip):

I actually forgot to check -- does the FCW direct to a reasonable landing issue?

pnkfelix (Jul 09 2019 at 15:03, on Zulip):

Do you mean in this context?

pnkfelix (Jul 09 2019 at 15:03, on Zulip):

or about future-compat warning general?

pnkfelix (Jul 09 2019 at 15:03, on Zulip):

for this specific lint about structural_match, there is this tracking issue I filed: #62411

pnkfelix (Jul 09 2019 at 15:04, on Zulip):

for general discussion of FCW issues, the best thing I can currently think of is issue #34596 (and PR #59658 for some angry debate!)

pnkfelix (Jul 09 2019 at 15:04, on Zulip):

I do rather like the example I picked for #62411. :)

nikomatsakis (Jul 09 2019 at 15:05, on Zulip):

for this specific lint about structural_match, there is this tracking issue I filed: #62411

looks great :100:

pnkfelix (Jul 12 2019 at 10:45, on Zulip):

hey @nikomatsakis , someone already pointed a somewhat disappointing oversight on my part: "indirect_structural_match lint misfiring on enum variant unused in const item" #62614

pnkfelix (Jul 12 2019 at 10:46, on Zulip):

what do you think about just reverting PR #62339, and I'll come back to doing it the right way in September? (See also more of my thoughts on this here)

nikomatsakis (Jul 12 2019 at 10:47, on Zulip):

I do not consider that a misfire

nikomatsakis (Jul 12 2019 at 10:47, on Zulip):

But I am ok with reverting the PR

pnkfelix (Jul 12 2019 at 10:47, on Zulip):

Really?

nikomatsakis (Jul 12 2019 at 10:47, on Zulip):

Well, ok, wait

nikomatsakis (Jul 12 2019 at 10:47, on Zulip):

maybe I am too hasty

pnkfelix (Jul 12 2019 at 10:47, on Zulip):

I guess it depends on what one's expectations are about a const

nikomatsakis (Jul 12 2019 at 10:47, on Zulip):

I guess it fits the intersection

pnkfelix (Jul 12 2019 at 10:47, on Zulip):

e.g. if we are worried with the const itself changing in the future

nikomatsakis (Jul 12 2019 at 10:47, on Zulip):

That is, the thing defines Eq, so in the "just use Eq" model, it would work

pnkfelix (Jul 12 2019 at 10:47, on Zulip):

to use the other variant

nikomatsakis (Jul 12 2019 at 10:47, on Zulip):

It also fits the "convert to a pattern" rule

nikomatsakis (Jul 12 2019 at 10:48, on Zulip):

(though how that works in generics, I have no idea, etc)

pnkfelix (Jul 12 2019 at 10:49, on Zulip):

Yeah, like I tried to say above: The only scenario where i could see value in the current warning, is if someone wanted to be forewarned if they were thinking they might change their const item to use the non-structural-variant.

nikomatsakis (Jul 12 2019 at 10:49, on Zulip):

what do you think about just reverting PR #62339, and I'll come back to doing it the right way in September? (See also more of my thoughts on this here)

basically I think @pnkfelix that it's waited this long, it can wait longer

pnkfelix (Jul 12 2019 at 10:50, on Zulip):

Yeah that was what I was thinking too

nikomatsakis (Jul 12 2019 at 10:50, on Zulip):

and I see the "I'm not going to be around to fix this" argument for sure :)

pnkfelix (Jul 12 2019 at 10:50, on Zulip):

the whole "do no harm" kind of thinking

pnkfelix (Jul 12 2019 at 10:50, on Zulip):

I'll put up a reversion PR

pnkfelix (Jul 12 2019 at 10:53, on Zulip):

actually, maybe I can just downgrade the lint for now to allow?

pnkfelix (Jul 12 2019 at 10:53, on Zulip):

/me thinks

nikomatsakis (Jul 12 2019 at 10:53, on Zulip):

plausible

pnkfelix (Jul 12 2019 at 10:53, on Zulip):

I don't actually want to undo all of the PR. In particular, it actually fixed ICE's

pnkfelix (Jul 12 2019 at 10:59, on Zulip):

hmm, weirdly, matthewjasper had warned me about this case when they said const A: &[Option<NoDerive>]; did I not make a test specifcally for that ...?

pnkfelix (Jul 12 2019 at 11:01, on Zulip):

Oh, I see, @Matthew Jasper may or may have been warning me about this case. They left the RHS as ... in their comment

Last update: Nov 16 2019 at 02:30UTC