unused type lint - #2552
Conversation
some people are saying its even better than unused local
| CHECK_EQ(result.warnings[0].location, Location(Position(1, 0), Position(3, 3))); | ||
| } | ||
|
|
||
| TEST_CASE_FIXTURE(Fixture, "unused_type_unrelated_global_reference") |
There was a problem hiding this comment.
testing for global access seems a little sparse, i should probably improve this
There was a problem hiding this comment.
I agree
next time please quote reply; i was very confused to see an "i agree" on this line until i visited the diff page :P will do when i get the chance though
EDIT: github mobile ui was bugged. classic.
EDIT 2: oh, no. it just didnt show the convo because it was displaying the comment in the review section haha
| if (context.warningEnabled(LintWarning::Code_TypeUnused)) | ||
| LintUnusedType::process(context); | ||
|
|
There was a problem hiding this comment.
whoops sorry will do
| CHECK_EQ(result.warnings[0].location, Location(Position(1, 0), Position(3, 3))); | ||
| } | ||
|
|
||
| TEST_CASE_FIXTURE(Fixture, "unused_type_unrelated_global_reference") |
|
|
||
| void report() | ||
| { | ||
| for (auto& pair : refs) |
There was a problem hiding this comment.
Minor nit: if you are going to iterate over it, prefer some sort of ordered data structure. You can compose a DenseHashSet with a std::vector, for example.
| void scopePush(Alias& alias) | ||
| { | ||
| alias.scopeDepth++; | ||
| typesEnvScopeStack.push_back(&alias); | ||
| } |
There was a problem hiding this comment.
nit: just have this take a NotNull<Alias> instead.
There was a problem hiding this comment.
n-not.. null? but i'm nnullcolumn..
sounds good though, will do when i get back to this PR (probably not today)
| Alias& alias = refs[node->name]; | ||
| alias.declaration = node; | ||
| alias.nameLocation = node->nameLocation; | ||
| alias.exported = node->exported; |
There was a problem hiding this comment.
nit, I think house style prefers just using emplace_back or try_insert.
| Alias& alias = refs[node->name]; | ||
| alias.declaration = node; | ||
| alias.nameLocation = node->nameLocation; | ||
| alias.exported = node->exported; | ||
|
|
| AstStat* declaration = nullptr; | ||
| unsigned int scopeDepth = 0; | ||
| bool referenced = false; | ||
| bool softReferenced = false; |
There was a problem hiding this comment.
Ditto as the other PR re "softReferenced"
| unsigned int scopeDepth = 0; | ||
| bool referenced = false; | ||
| bool softReferenced = false; | ||
| bool exported = false; |
There was a problem hiding this comment.
Why not rename referenced to referencedOrExported?
There was a problem hiding this comment.
i agree; wasn't super satisfied with that naming anyways
|
hopefully going to have time to come back to this soon. just thought of another test case, i should definitely be looking out for recursive type aliases |
2 participants
unused type alias / type function lint pass. does not trigger on
export, references to type functions from within that type function's body don't count. tested failure cases pretty extensively, pr/code/tests are 100% human authored.silencing path is to prefix the type with
_for now. Although you cannot declare multiple type aliases with the name_, unused types like that are niche, and users can always--!nolint TypeUnused.