-
Notifications
You must be signed in to change notification settings - Fork 664
feat(rome_analyze): implement the validTypeof rule #2649
Conversation
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
|
Test result | main count |
This PR count | Difference |
---|---|---|---|
Total | 588 | 588 | 0 |
Passed | 519 | 519 | 0 |
Failed | 69 | 69 | 0 |
Panics | 0 | 0 | 0 |
Coverage | 88.27% | 88.27% | 0.00% |
ts/microsoft
Test result | main count |
This PR count | Difference |
---|---|---|---|
Total | 16257 | 16257 | 0 |
Passed | 12391 | 12391 | 0 |
Failed | 3866 | 3866 | 0 |
Panics | 0 | 0 | 0 |
Coverage | 76.22% | 76.22% | 0.00% |
Deploying with Cloudflare Pages
|
|
||
pub(crate) enum ValidTypeof {} | ||
|
||
impl Rule for ValidTypeof { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some documentation of what the rule is testing for.
ActionCategory, RuleCategory, | ||
}; | ||
|
||
pub(crate) enum ValidTypeof {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty enum... that's interesting. What's the reason behind using an empty enum over struct ValidTypeof;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make a huge difference, we just need a type to implement the Rule
trait on but that type will never be instantiated and this is made clear by using a type that actually can't be instantiated at all (while an empty struct can be instantiated, but will be zero-sized)
.trim_start_matches(['"', '\'']) | ||
.trim_end_matches(['"', '\'']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do literal.inner_string_text()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inner_string_text
returns a SyntaxNodeText
, which I found less practical in this case as it needs to be collected into an allocated String
in order to be matched-on, while text_trimmed
returns an &str
directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no current usages of it. Should we change the implementation to return a string slice instead? Sounds like it's the more practical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that would make sense, the only downside I see to that is that a &str
slice doesn't have any associated range information but it shouldn't be too much of a problem
return None; | ||
} | ||
|
||
if let JsAnyLiteralExpression::JsStringLiteralExpression(literal) = literal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the right-hand side is a template literal?
>> typeof "a" === `string`
true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently unsupported, I could probably add some code to handle the simple case with a single chunk / no interpolation, but it may not be needed eventually depending on how this rule interacts with the noUnusedTemplateLiteral rule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule should follow the naming convention: https://github.com/rome/tools/blob/main/CONTRIBUTING.md#naming-patterns
Hence, it should be called useValidTypeof
or noInvalidTypeof
Some(RuleDiagnostic { | ||
severity: Severity::Error, | ||
message: markup! { | ||
"Invalid typeof comparison value" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should tell why the comparison is invalid.
"Invalid typeof comparison value" | |
"Invalid `typeof` comparison value" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make a note on diagnostics here, in the context of a Rust-like diagnostic with rich codespan information it would be possible to attach context-specific messages to the actual label:
error[useValidTypeof]: Invalid typeof comparison value
┌─ useValidTypeof.js:10:1
│
10 │ typeof bar !== "fucntion"
│ ^^^^^^^^^^ not a valid type name
error[useValidTypeof]: Invalid `typeof` comparison value
┌─ useValidTypeof.js:14:1
│
14 │ typeof bar == Object
│ ^^^^^^ not a string literal
However I'm not entirely certain how to represent this information in the context of the LSP where we have a lot less control on how these diagnostics are rendered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we should provide two messages/diagnostics: one for the CLI/CI and one for the LSP
a303b42
to
3933738
Compare
Summary
This PR implements the valid-typeof rule from ESLint verifying the result of
typeof
unary expressions is being compared to a valid expression (either a string literal that's a valid type name or anothertypeof
expression)Test Plan
I added test cases for the rule based on the examples found in the ESLint docs