incr compilation struct_defs.rs

[vitiral]

I am prematurely openeing this as I need mentoring help from @michaelwoerister (also pinged @nikomatsakis)

First, is this the right approach for these changes?

Second, I'm a bit confused by the results so far.

• Changing TupleStructFieldType(i32) -> ...(u32) changes only Hir and HirBody, not TypeOfItem
• Chaning TupleStructAddField(i32) -> ...(i32, u32) does change TypeOfItem

This seems wrong. I feel like it should change TypeOfItem in both cases. Is this a bug in incr compilation or is it expected?

[vitiral]

link to #44924

[vitiral]

even more confusing, RecordStructFieldType (which changes internal type from f32 -> u64) doesn't cause TypeOfItem to change

[vitiral]

another red flag, this one is probably correct:

AddLifetimeParameterBound: adding lifetime bound does NOT change type/generics, but does change Predicates

[vitiral]

I completed the tests in the file so that they all pass. Had several "unsure" etc questions, which I marked in the commit messages (as well as the questions I had here).

There is also one outstanding FIXME. I'm not sure what to do with the EmptyStruct -- it seems weird that it works at all, since it doesn't follow the same pattern as the others.

[michaelwoerister]

• You are running into what I briefly described in incr.comp.: Update fingerprint-based auto tests for red/green tracking. #44924: Fields of structs, tuples, enums, and unions are separate from their containing type. The Ty value that represents a struct (for example) only contains a reference to a Field value, which has its own type. So changing the type of the field, results in a changed fingerprint for the field but not the containing type. This is why there are also annotations on each of the fields, e.g.:
  

  rust/src/test/incremental/hashes/struct_defs.rs

  Lines 67 to 71 in 2146c61

  	struct TupleStructFieldType(
  	#[rustc_metadata_dirty(cfg="cfail2")]
  	#[rustc_metadata_clean(cfg="cfail3")]
  	u32
  	);

  
  If the name or visibility of a field changes, or if a field gets added or removed, then the TypeOfItem of the containing type should change.

• Something similar is going on in the AddLifetimeParameterBound case. As long as the PredicatesOfItem fingerprint changes, this is correct.

• Since we cannot change anything in the EmtpyStruct case, we just make sure that the fingerprint is stable (i.e. that there are no random influences like memory addresses taken into account by the hashing algorithm). This is what we normally do between cfail2 and cfail3.

[michaelwoerister]

Btw, opening a PR before it is completely done is common practice here and a good way to get feedback early.

[vitiral]

So changing the type of the field, results in a changed fingerprint for the field but not the containing type.

Is there a way to assert specific DepNodeKinds haven't changed for fields in rustc_metadata? If there is we should be doing that as well.

... EmptyStruct

I'm going to add a comment, because on its own it looks wrong.

Thanks!

[vitiral]

So the thing I am most confused about for EmptyStruct is... shouldn't it be this:

#[cfg("cfail1")]
pub struct EmptyStruct;

#[rustc_clean(label="Hir", cfg="cfail2")]
#[rustc_metadata_clean(cfg="cfail2")]
pub struct EmptyStruct;

I still don't understand how something that didn't exist in the previous compilation session can have a "clean" Hir! Is it "clean" when it is being first defined?

[michaelwoerister]

So the thing I am most confused about for EmptyStruct is...

Note that the definition does not have a #[cfg] attribute, so it will exist in all sessions.

[carols10cents]

r? @michaelwoerister

[vitiral]

removed FIXME and rebased

[vitiral]

@michaelwoerister that should pass and be good to go

[michaelwoerister]

Thanks, @vitiral! This looks very good. I could not spot anything wrong.

Some comments:

• Is there a reason that the change_trait_bound_indirectly case is left out?
• The EmptyStruct case should be extended with the other struct labels too.
• Struct fields are not checked yet, but it looks like the testing infrastructure doesn't support that yet, so it's something for another PR.

Once these comments are addressed, this is good to go!

[michaelwoerister]

Could you add the other labels here too? All of them should be unchanged.

[arielb1]

If the name or visibility of a field changes, or if a field gets added or removed, then the TypeOfItem of the containing type should change.

Why is that (with red/green)? The TypeOfItem of a struct is just a Struct<Param1, Param2, ...>, which has no reason to change when fields are touched.

[michaelwoerister]

@arielb1: TypeVariants::TyAdt gives direct access to the AdtDef, so we have to incorporate its contents into the hash.

[vitiral]

@michaelwoerister I ammended the commit with your changes. I also made assertions on EmptyStruct for cfail3 as well.

[michaelwoerister]

@bors r+

[bors]

📌 Commit c5c3614 has been approved by michaelwoerister

[michaelwoerister]

@vitiral 👍

[bors]

⌛ Testing commit c5c3614 with merge 4c25acc721cfba42466eea531d804b3d6208fb43...

[bors]

💔 Test failed - status-travis

[michaelwoerister]

@bors retry
Looks like spurious error #43402.

[vitiral]

@bors retry

looks like it failed again?

[bors]

@vitiral: 🔑 Insufficient privileges: and not in try users

[michaelwoerister]

It's still waiting in the queue: https://buildbot2.rust-lang.org/homu/queue/rust

[vitiral]

gotcha, man it takes a long time to run :) I see that several commits are "rolled up" together though, so that is good.

[michaelwoerister]

Yes, it can take quite a while until a change lands.

[bors]

⌛ Testing commit c5c3614 with merge b915820...

[bors]

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