Conversation
Signed-off-by: Ashley Claymore <aclaymore@bloomberg.net>
tchetwin
left a comment
There was a problem hiding this comment.
Unfinished review, nits/questions only
Would appreciate some more inline comments of the syntactical structures involves in functions like hasUnsafeNullishLogicalMix
| const tsInput = `1 + 1 as unknown /* comment */ / 2`; | ||
| const jsOutput = tsBlankSpace(tsInput, onError); | ||
| assert.equal(onError.mock.callCount(), 1); | ||
| assert.equal(jsOutput, tsInput); |
There was a problem hiding this comment.
turbo-nit: in the other fixtures the tsInput variable is not reused, the input is duplicated
| if (!tslib.isBinaryExpression(baseExpr)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Do we need to consider the ternary operator?
There was a problem hiding this comment.
I don't think we need to consider the case where the base is a ternary because:
true ? a : b as number / 2Is parsed as:
true ? a : (b as number / 2);Not:
(true ? a : b as number) / 2;So removing the as number does not change how the expression is parsed.
For the ternary to be the base it would of had to be written as:
(true ? a : b) as number / 2Where as number is still safe to erase.
|
|
||
| // Other safe targeted cases: | ||
| 1 * 1 as unknown + 2; | ||
| 1 ?? 1 as unknown || 2; |
There was a problem hiding this comment.
This is not safe; both TypeScript and the JavaScript engine require parentheses.
Because the input code already throws an error, I’m not sure whether this complies with the garbage in, garbage out principle.
There was a problem hiding this comment.
"I’m not sure whether this complies with the garbage in, garbage out principle."
Exactly that. There are existing cases in ts-blank-space where the emitted code is invalid, but that's because the input was invalid in the first place.
That said, I'll update this fixture as this fixture should only be testing valid inputs. Thanks for catching.
|
|
||
| it("errors on `as` expression that would change operator precedence", () => { | ||
| const onError = mock.fn(); | ||
| const tsInput = `1+1 as T / 2`; |
There was a problem hiding this comment.
There is another case,
1 + 1 as const / 2;Although it looks like a type assertion (as T), it is actually a const assertion.
The TypeScript compiler reports a type error, but the underlying codegen can still produce valid JavaScript by adding parentheses to ensure the correct order of operations.
It is unclear whether this case should be handled explicitly.
There was a problem hiding this comment.
Good point. I've added logic in 71ffc11 so that this is a build error too.
Signed-off-by: Ashley Claymore <aclaymore@bloomberg.net>
Signed-off-by: Ashley Claymore <aclaymore@bloomberg.net>
Signed-off-by: Ashley Claymore <aclaymore@bloomberg.net>
Issue number of the reported bug or feature request: #62
Describe your changes
This PR adds new logic to emit an error when a type assertion can not be safely erased.
Testing performed
New test fixtures for both good and bad cases have been added.
Additional context
It would be simpler to not take the operator precedence into account and to always error instead, forcing the author to always use explicit parenthesis when mixing type assertion operators within binary expressions. This felt too restrictive and adding operator precedence checks is not too complex.