Improve assert message in binder #38270
Conversation
Looking at the code, I don't think the assert can ever fire, but it clearly does, or did in the past. This will make it easier for people to create a repro.
|
The diff is 2x easier to read with ignore-whitespace on, which is sad for a 10-line diff. |
src/compiler/binder.ts
Outdated
| else { | ||
| bindStaticPropertyAssignment(cast(node.left, isBindableStaticAccessExpression)); | ||
| if (!isBindableStaticAccessExpression(node.left)) { | ||
| return Debug.fail(`Invalid cast. The supplied value ${getTextOfNode(node.left)} was not a BindableStaticAccessExpression.`); |
There was a problem hiding this comment.
Couldn’t this potentially put sensitive information in telemetry?
There was a problem hiding this comment.
yes, but does this go to telemetry?
There was a problem hiding this comment.
I think so, via the editors? /cc @amcasey
There was a problem hiding this comment.
Correct: this would end up in telemetry and should not. Would the node kind suffice?
There was a problem hiding this comment.
p.s. Thanks for thinking about this @andrewbranch
There was a problem hiding this comment.
@amcasey it's unlikely to help, since the predicates are very complicated. It's likely the kind of some deeply nested node that's wrong.
This type does allow identifiers, but those are ruled out earlier, so I added an assert for that case.
|
So....here's a different idea. Just use the same predicate that getAssignmentDeclarationKind does, which should avoid the assert. We should only take this if we can get a repro from the bug, though. It doesn't break any existing tests, but we already know that coverage isn't very good. |
|
Should we try to put this into 3.9 so that we can pinpoint the issue sooner? |
| } | ||
| } | ||
|
|
||
| /** | ||
| * For nodes like `x.y = z`, declare a member 'y' on 'x' if x is a function (or IIFE) or class or {}, or not declared. | ||
| * Also works for expression statements preceded by JSDoc, like / ** @type number * / x.y; | ||
| */ | ||
| function bindStaticPropertyAssignment(node: BindableStaticAccessExpression) { | ||
| function bindStaticPropertyAssignment(node: BindableStaticNameExpression) { | ||
| Debug.assert(!isIdentifier(node)); |
There was a problem hiding this comment.
| Debug.assert(!isIdentifier(node)); | |
| Debug.assertNotNode(node, isIdentifier); |
|
@typescript-bot cherry-pick this to release-3.9 |
|
Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into |
|
Hey @DanielRosenwasser, I've opened #38434 for you. |
Component commits: 9795fa6 Improve assert message in binder Looking at the code, I don't think the assert can ever fire, but it clearly does, or did in the past. This will make it easier for people to create a repro. d815eff fix lint add6bbc Use BindableStaticNameExpression not BindableStaticAccessExpression This type does allow identifiers, but those are ruled out earlier, so I added an assert for that case.
🤖 Pick PR #38270 (Improve assert message in binder) into release-3.9
|
Well, since this is in 3.9, I'm going to merge it into master as well and Hope For The Best. |
* upstream/master: (54 commits) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Fix for jsdoc modifiers on constructor params (microsoft#38403) Improve assert message in binder (microsoft#38270) fix broken regex on "src/services/completions.ts#getCompletionData" (microsoft#37546) report error for duplicate @type declaration (microsoft#38340) fix(38073): hide 'Extract to function in global scope' action for arrow functions which use 'this' (microsoft#38107) Update user baselines (microsoft#38472) Update user baselines (microsoft#38405) Changed template strings to emit void 0 instead of undefined (microsoft#38430) Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit (microsoft#38368) LEGO: check in for master to temporary branch. Make isDynamicFileName available publicly (microsoft#38269) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Exclude arrays and tuples from full intersection property check (microsoft#38395) Fix crash caused by assertion with evolving array type (microsoft#38398) Update user baselines (microsoft#38128) LEGO: check in for master to temporary branch. moveToNewFile: handle namespace imports too ... # Conflicts: # src/compiler/types.ts # src/compiler/utilities.ts
Looking at the code, I don't think the assert can ever fire, but it clearly does, or did in the past. This will make it easier for people to create a repro for #37633.