[Compiler-V2] Unused type parameter checking #12806
Conversation
fEst1ck
changed the title
brmataptos
changed the title
brmataptos
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12806 +/- ##
===========================================
- Coverage 69.2% 62.6% -6.7%
===========================================
Files 2296 822 -1474
Lines 438064 184232 -253832
===========================================
- Hits 303482 115435 -188047
+ Misses 134582 68797 -65785 ☔ View full report in Codecov by Sentry. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| let mut set = BTreeSet::new(); | ||
| set.insert(*i); | ||
| set | ||
| }, |
There was a problem hiding this comment.
| let mut set = BTreeSet::new(); | |
| set.insert(*i); | |
| set | |
| }, | |
| BTreeSet::from([*i]) | |
| }, |
There was a problem hiding this comment.
You could also use BTreeSet::from_iter(iter::once(*i))
| set | ||
| }, | ||
| Type::Fun(from, to) => { | ||
| let mut used_in_from = used_type_parameters_in_ty(from); |
There was a problem hiding this comment.
Maybe nice to have functional style code here instead: used_type_parameters_in_ty(from).union(&used_type_parameters_in_ty(to)).collect() or similar? It seems like we have mixed functional (with the flat_map above) and imperative styles, which seems a bit inconsistent.
There was a problem hiding this comment.
I didn't do it this way because this would require copies in general. (But copying u16 is ok in this case).
| .collect() | ||
| } | ||
|
|
||
| /// Returns the indices of type parameters used in the given type. |
There was a problem hiding this comment.
Are these indices unique across all possible type parameters in the program, or within a struct declaration, or something else?
When I read this comment, it made me think the indices are w.r.t the given type, but that could be problematic. Maybe we want to clarify what the "scope" of these indices are (eg., unique across all compilation targets): ideally, this would be done where TypeParameter is declared through a comment.
There was a problem hiding this comment.
The indices are not unique cross programs, structs, etc. Only the caller of the method may know the scope, but we can be sure that the indices within a type are in the same scope
There was a problem hiding this comment.
Could you please add a comment of this nature for this function documentation? For example, if have a vector inside a struct, the "given type" when calling this method recursively would be the vector, but the type param indexes refer to the original type struct.
| set.insert(*i); | ||
| set | ||
| }, | ||
| Type::Fun(from, to) => { |
There was a problem hiding this comment.
Is this code even reachable?
There was a problem hiding this comment.
Currently no. I include this case in case we allow them in the future.
There was a problem hiding this comment.
Sorry, which test case triggers this (or attempts to)? I see the ones for other features, but not this one. I am not sure we have a plan to ever have functions as fields of structs.
In any case, I think we should move it to the unreachable! case because I don't understand the interaction of phantom types with function types in structs, and we should not have arbitrary implementations.
| // Copyright © Aptos Foundation | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| //! Implements an environment pipeline which checks for unused type parameters in struct definitions. |
There was a problem hiding this comment.
Would be good to add if there are any pre-conditions (such as passes and checks that this assumes), so that we can re-order stuff.
Eg, we may require recursive struct checks to be already done, otherwise, we may have infinite recursion here.
There was a problem hiding this comment.
Added pre-conditions. (But we don't need recursive struct checks in this case.)
| fn used_type_parameters_in_ty(ty: &Type) -> BTreeSet<u16> { | ||
| match ty { | ||
| Type::Primitive(_) => BTreeSet::new(), | ||
| Type::Tuple(tys) | Type::Struct(_, _, tys) => { |
There was a problem hiding this comment.
Can tuple types be reachable in this context?
There was a problem hiding this comment.
Currently no. I include this case in case we allow them in the future.
There was a problem hiding this comment.
Here, as well as in all other cases that are not currently reachable, we should instead move them to the unreachable! case below. I think we should not speculate non-existent features and invent an unnecessary implementation.
| used_in_from.append(&mut used_type_parameters_in_ty(to)); | ||
| used_in_from | ||
| }, | ||
| Type::Vector(ty) | Type::Reference(_, ty) => used_type_parameters_in_ty(ty), |
There was a problem hiding this comment.
Are reference types reachable in this context?
There was a problem hiding this comment.
Currently no. I include this case in case we allow them in the future.
There was a problem hiding this comment.
Also, let's please move them to unreachable!, so if we ever have this in the future, we are forced to think about its semantics and implications.
| struct S2<T1, phantom T2> { | ||
| f: S3<T2>, | ||
| } | ||
|
|
There was a problem hiding this comment.
Could we add some more tests for coverage here (or in a different test file)?
- use vectors, maybe some more depth of structs, other stuff asked I asked for reachability if they are reachable?
- add recursive structs with type params (to perhaps catch any unwanted re-orderings of passes).
|
All comments addressed. PTAL @vineethk |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
Description
Fixed #11709
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Existing tests and
checking/naming/unused_type_parameter_struct.move