incr.comp.: Create Test Case for Incr. Comp. Hash for traits

[michaelwoerister]

This issue is part of a broader effort to implement comprehensive regression testing for incremental compilation. For the tracking issue go to #36350.

Background

For incremental compilation we need to determine if a given HIR node has changed in between two versions of a program. This is implemented in the calculate_svh module. We compute a hash value for each HIR node that corresponds to a node in the dependency graph and then compare those hash values. We call this hash value the Incremental Compilation Hash (ICH) of the HIR node. It is supposed to be sensitive to any change that might make a semantic difference to the thing being hashed.

Testing Methodology

The auto-tests in the src/test/incremental directory all follow the same pattern:

• Each source file defines one test case
• The source file is compiled multiple times with incremental compilation turned on, each time with a different --cfg flag, allowing each version to differ via conditional compilation. Each of these versions we call a revision.
• During each revision, the test runner will make sure that some assertions about the dependency graph of the program hold.
• These assertions are specified in the source code of the test file as attributes attached to the items being tested (e.g. #[rustc_clean]/#[rustc_dirty]).

The ICH-specific tests use this framework by adhering to the following pattern:

• There are two versions of the definition under test, one marked with #[cfg(cfail1)] and the second marked with #[cfg(not(cfail1))]. As a consequence, the first revision will be compiled with the first version of the definition, and all other revisions will be compiled with the second version. The two versions are supposed to differ in exactly one aspect (e.g. the visibility of a field is different, or the return of a function has changed).
• The second definition has a #[rustc_dirty(label="Hir", cfg="cfail2")] attribute attached. This attribute makes the test runner check that a change of the Hir dependency node of the definition has been detected between revisions cfail1 and cfail2. This will effectively test that a different ICH value has been computed for the two versions of the definition.
• The second definition also has a #[rustc_clean(label="Hir", cfg="cfail3")] attribute. This makes sure that the Hir dependency node (and thus the ICH) of the definition has not changed between revisions cfail2 and cfail3. That's what we expect, because both revisions use the same version of the definition.
• For definitions that are exported from the crate, we also want to check the ICH of the corresponding metadata. This is tested using the #[rustc_metadata_clean]/#[rustc_metadata_dirty] attributes and works analogous to the Hir case: We add #[rustc_metadata_dirty(cfg="cfail2")] to the second definition to make sure that the ICH of the exported metadata is not the same for the different versions of the definition, and we add #[rustc_metadata_dirty(cfg="cfail3")] to make sure that the ICH is the same for the two identical versions of the definition.

Why are the revisions called "cfail"? That's because of reasons internal to how
the test-runner works. Prefixing a revision with "cfail" tells the test runner to treat the test like a "compile-file" test, that is: compile the test case but don't actually run it (which would be the case for an "rpass" revision). For the ICH tests we need to compile "rlibs", so that we can test metadata ICHs. As a consequence we cannot declare them "rpass". In an additional directive (// must-compile-successfully), we tell the test runner that we actually expect the file to not produce any errors.

Each test case must contain the following test-runner directives and crate attributes at the top:

// must-compile-successfully
// revisions: cfail1 cfail2 cfail3
// compile-flags: -Z query-dep-graph

#![feature(rustc_attrs)]  // <-- let's us use #[rustc_dirty], etc.
#![crate_type="rlib"]     // <-- makes sure that we export definitions

See src/test/incremental/hashes/struct_defs.rs for a full example of such a ICH regression test.

Trait Specific ICH Testing

Each of the following things should be tested with its own definition pair:

• Change trait visibility
• Change trait unsafety
• Add method
• Change name of method
• Add return type to method
• Change return type of method
• Add parameter to method
• Change name of method parameter
• Change type of method parameter (i32 => i64)
• Change type of method parameter (&i32 => &mut i32)
• Change order of method parameters
• Add default implementation to method
• Change order of methods
• Change mode of self parameter
• Add unsafe modifier to method
• Add extern modifier to method
• Change extern "C" to extern "rust-intrinsic"
• Add type parameter to method
• Add lifetime parameter to method
• Add trait bound to method type parameter
• Add builtin bound to method type parameter
• Add lifetime bound to method type parameter
• Add second trait bound to method type parameter
• Add second builtin bound to method type parameter
• Add second lifetime bound to method type parameter
• Add associated type
• Add trait bound to associated type
• Add lifetime bound to associated type
• Add default to associated type
• Add associated constant
• Add initializer to associated constant
• Change type of associated constant
• Add super trait
• Add builtin bound (Send or Copy)
• Add 'static lifetime bound to trait
• Add super trait as second bound
• Add builtin bound as second bound
• Add 'static bounds as second bound
• Add type parameter to trait
• Add lifetime parameter to trait
• Add trait bound to type parameter of trait
• Add lifetime bound to type parameter of trait
• Add lifetime bound to lifetime parameter of trait
• Add builtin bound to type parameter of trait
• Add second type parameter to trait
• Add second lifetime parameter to trait
• Add second trait bound to type parameter of trait
• Add second lifetime bound to type parameter of trait
• Add second lifetime bound to lifetime parameter of trait
• Add second builtin bound to type parameter of trait
• Add trait bound to type parameter of trait in where clause
• Add lifetime bound to type parameter of trait in where clause
• Add lifetime bound to lifetime parameter of trait in where clause
• Add builtin bound to type parameter of trait in where clause
• Add second type parameter to trait in where clause
• Add second lifetime parameter to trait in where clause
• Add second trait bound to type parameter of trait in where clause
• Add second lifetime bound to type parameter of trait in where clause
• Add second lifetime bound to lifetime parameter of trait in where clause
• Add second builtin bound to type parameter of trait in where clause

EDIT: Some more cases

• Change return type of method indirectly by modifying a use statement
• Change type of method parameter indirectly by modifying a use statement
• Change trait bound of method type parameter indirectly by modifying a use statement
• Change trait bound of method type parameter in where clause indirectly by modifying a use statement
• Change trait bound of trait type parameter indirectly by modifying a use statement
• Change trait bound of trait type parameter in where clause indirectly by modifying a use statement

[michaelwoerister]

cc @nikomatsakis

[eulerdisk]

@michaelwoerister I would like to take this one too. It will take a little longer this time 😐

[michaelwoerister]

Sure, go ahead! There's no rush either, you can take your time.

[eulerdisk]

@michaelwoerister
Ok, I will finish soon the tests (some of theme fail, I'll do a list after I re-check them..).
Before to push I have some annotations/questions.

• "Add second type parameter to trait in where clause" don't make sense, or where you talking about a second bound ?
• #[rustc_metadata_dirty(cfg="cfail3")] is missing from the "change visibility" test both in enum and struct tests.
  Is that correct or a slip ?
• For the "indirect" tests, other than changing the USE statement, I added another variant of the same tests that change the type using a TYPE statement. I don't know if that make sense to you but they all seem to fail.
  If so I will update the enum tests I did last week to include such variants.
  Example:

// Change return type of method indirectly by modifying a type statement------------
mod change_return_type_of_method_indirectly_type {
    #[cfg(cfail1)]
    type ReturnType = super::ReferenceType0;
    #[cfg(not(cfail1))]
    type ReturnType = super::ReferenceType1;

    #[rustc_dirty(label="Hir", cfg="cfail2")]
    #[rustc_clean(label="Hir", cfg="cfail3")]
    #[rustc_metadata_dirty(cfg="cfail2")]
    #[rustc_metadata_clean(cfg="cfail3")]
    trait TraitChangeReturnType {
        fn method() -> ReturnType;
    }
}

[michaelwoerister]

"Add second type parameter to trait in where clause" don't make sense, or where you talking about a second bound ?

You're right. I removed them from the list.

#[rustc_metadata_dirty(cfg="cfail3")] is missing from the "change visibility" test both in enum and struct tests. Is that correct or a slip ?

It had something to do with non-public items not being exported and thus not being present in metadata.

For the "indirect" tests, other than changing the USE statement, I added another variant of the same tests that change the type using a TYPE statement. I don't know if that make sense to you but they all seem to fail.

That's a very good point. The test would make sense, if the compiler treats type declarations more or less as transparent (like it does with use statements). I'll get back to you about that. But it's an excellent observation in any case 👍

[michaelwoerister]

So, it is OK that changing the type alias does not change the hash of items referencing the alias. That change is detected later during analysis when the alias is resolved. I've added a test case making sure that's really the case: #36932