-
-
Notifications
You must be signed in to change notification settings - Fork 504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(lint/noUnreachableSuper): handle complex CFG #97
Conversation
8dce8e0
to
a720e75
Compare
a720e75
to
698cde3
Compare
698cde3
to
523c15b
Compare
noUnreachableSuper and noInvalidConstructorSuper have covering cases. They both report classes that should call To avoid double reports, I suggest removing the common check from noUnreachableSuper. Another solution is merging the two rules into noUnreachableSuper. |
It's possible it's a bug |
crates/rome_js_analyze/src/analyzers/correctness/no_unreachable_super.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/analyzers/correctness/no_unreachable_super.rs
Show resolved
Hide resolved
@ematipico |
I believe they should be two different rules, they have clearly two different purposes and they don't necessarily overlap. It's totally reasonable to have two rules that report on the same |
e532ebb
to
059ebaf
Compare
Summary
Fix rome#4616.
I take the opportunity to fix an unnoticed bug: the rule assimilated super calls such as
super.method()
to a super callsuper()
.I think that the new implementation is simpler and more robust.
i discovered some cases statically undecidable. For instance the following example:
As a workaround, I decided to assume that the
try
block has both asuper()
call and nosuper()
call when an exception is caught. Thus, the rule reports duplicatesuper()
in the previous example.We could also ignore
catch
blocks. Any thought?EDIT: TypeScript uses the same workaround.
Note: I have just noticed that I had a wrong assumption about throw expressions. Biome CFG wrapped every
throw
expression in a return instruction, includingthrow
in atry catch
block. I expected a jump for this special case... Not sure if this should be fixed in the rule implementation or on the CFG?I think this issue requires more thoughts and should be fixed in another PR.
Test Plan
New tests included.