Flag middle comma SEF operands#33648
Conversation
|
@typescript-bot test this |
|
Heya @RyanCavanaugh, I've started to run the parallelized community code test suite on this PR at d851fd2. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at d851fd2. You can monitor the build here. It should now contribute to this PR's status checks. |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
| // A comma operator is SEF if either operand is SEF, e.g. the template argument in | ||
| // `The coordinates are ${x.toString(), y, z}` | ||
| // contains an illegally SEF expression at 'y' (the left side of the outer comma whose right operand is 'z') | ||
| return isSideEffectFree((node as BinaryExpression).left) || isSideEffectFree((node as BinaryExpression).right); |
There was a problem hiding this comment.
So this is less isSideEffectFree and more containsSideEffectFree now, right? Since technically the comma operator itself would only actually be side-effect free if everything it executed was free of side effects?
There was a problem hiding this comment.
I don't think so. f() + 4 contains a non-side-effect-free expression, but is side-effect-free due to the addition operator itself not being something that can produce side effects*
Upon thinking about this more, I think the correct fix is just to return true if the operator in a binary expression isn't an assignment operator. The expression [(f(), 4, 2)] is equally wrong as [(f() + 4, 2)]; the comma is a red herring. I'll try this on Monday.
* let's continue to pretend valueOf doesn't exist, per convention
|
@RyanCavanaugh it looks like you were intending to simplify this PR. Do you want to try that or should I just close it since it's been quite a while? |
4 participants
Fixes #33646