Add missing bounds check in isReachableFlowNodeWorker
#873
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a missing bounds check in isReachableFlowNodeWorker to prevent potential index out of range errors during flow analysis.
- Added a new test case for assertion with no argument.
- Updated baseline reference files for types and symbols.
- Modified internal/checker/flow.go to add a bounds check for predicate.parameterIndex.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| testdata/tests/cases/compiler/assertionWithNoArgument.ts | Adds a test function and test calls for assertion behavior. |
| testdata/baselines/reference/compiler/assertionWithNoArgument.types | Updates baseline for types to reflect the new assertion function. |
| testdata/baselines/reference/compiler/assertionWithNoArgument.symbols | Updates symbol baseline to include the new assertion function. |
| internal/checker/flow.go | Adds a bounds check for predicate.parameterIndex to prevent index out of range errors. |
internal/checker/flow.go
Outdated
| return false | ||
| if signature := c.getEffectsSignature(flow.Node); signature != nil { | ||
| if predicate := c.getTypePredicateOfSignature(signature); predicate != nil && predicate.kind == TypePredicateKindAssertsIdentifier && predicate.t == nil { | ||
| if arguments := flow.Node.Arguments(); int(predicate.parameterIndex) < len(arguments) { |
There was a problem hiding this comment.
The added bounds check prevents potential index out of range errors. Consider whether additional handling (e.g., logging) is needed if predicate.parameterIndex is unexpectedly out of bounds.
Copilot uses AI. Check for mistakes.
internal/checker/flow.go
Outdated
| if signature := c.getEffectsSignature(flow.Node); signature != nil { | ||
| if predicate := c.getTypePredicateOfSignature(signature); predicate != nil && predicate.kind == TypePredicateKindAssertsIdentifier && predicate.t == nil { | ||
| if arguments := flow.Node.Arguments(); int(predicate.parameterIndex) < len(arguments) { | ||
| if predicateArgument := arguments[predicate.parameterIndex]; predicateArgument != nil && c.isFalseExpression(predicateArgument) { |
There was a problem hiding this comment.
I don't think you need predicateArgument != nil once you have the bounds check.
| if predicateArgument := arguments[predicate.parameterIndex]; predicateArgument != nil && c.isFalseExpression(predicateArgument) { | |
| if predicateArgument := arguments[predicate.parameterIndex]; c.isFalseExpression(predicateArgument) { |
| assertWeird(); | ||
| assertWeird("hello"); |
There was a problem hiding this comment.
Might be worth having another test that swaps the order?
There was a problem hiding this comment.
Nah, isn't really relevant. See my comment below.
|
Any idea why two calls are necessary to trigger this crash? |
You just need some other statement after the argument-less call to trigger the situation. It's because in the call to |
Fixes #871.