[XS] Change in dart/sdk[main]: [dart2wasm] Fix unsound type-argument check optimization on covarianc...
Kevin Moore (Gerrit)
Kevin Moore added 1 comment![]( "Open in Gerrit")
Jackson: weird that you're an owner here? Shouldn't I be bothering Martin?
Related details
- Jackson Gardner
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Moore (Gerrit)
Kevin Moore voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kevin Moore
| Commit-Queue | +1 |
1 comment
FWIW: I'm surprised this has just been sitting here silently failing for...years?
Related details
- Jackson Gardner
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Moore (Gerrit)
Kevin Moore added 1 comment![]( "Open in Gerrit")
Ugh. Yes, that description was AI generated.
TL;DR: in my migration to bazel I hit this weird single crash in the wasm suite.
Had the agent search for existing issue. It existed.
Had the agent see if she could fix it. She did.
Existing failing test now passes.
ZERO clue if it's a good solution. I really don't understand.
But figured this was better than adding yet another special case in my weird branch for a thing that should just be fixed :shrug:
Related details
- Jackson Gardner
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
: operandType,Even though it is "safe" to do that, I don't think it's the right fix.
It completely ignores `operandType` and thereby ignores valuable information that makes the checks cheaper.
That can be locally fixed here by making use of parts of `operandType` that we know are safe to use and discarding other parts.
Though after thinking a bit more about this, I come to the conclusion that this is a symptom of a bigger issue and it would be best handled in the CFE instead.
I've talked to the CFE team and they seem to agree and will look into fixing it in a better way.
Related details
- Jackson Gardner
- Kevin Moore
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
: operandType,Even though it is "safe" to do that, I don't think it's the right fix.
It completely ignores `operandType` and thereby ignores valuable information that makes the checks cheaper.
That can be locally fixed here by making use of parts of `operandType` that we know are safe to use and discarding other parts.
Though after thinking a bit more about this, I come to the conclusion that this is a symptom of a bigger issue and it would be best handled in the CFE instead.
I've talked to the CFE team and they seem to agree and will look into fixing it in a better way.
If you really want to land it before handling this in the CFE, you can change this to use nullable/non-nullable top type in general (we can rely on the nullability), but if `operandType` is an interface, type, use it but defaults-to-bounds args.
i.e. re-write `operandType` to a safe version
Kevin Moore (Gerrit)
Kevin Moore added 1 comment![]( "Open in Gerrit")
: operandType,Martin KustermannEven though it is "safe" to do that, I don't think it's the right fix.
It completely ignores `operandType` and thereby ignores valuable information that makes the checks cheaper.
That can be locally fixed here by making use of parts of `operandType` that we know are safe to use and discarding other parts.
Though after thinking a bit more about this, I come to the conclusion that this is a symptom of a bigger issue and it would be best handled in the CFE instead.
I've talked to the CFE team and they seem to agree and will look into fixing it in a better way.
If you really want to land it before handling this in the CFE, you can change this to use nullable/non-nullable top type in general (we can rely on the nullability), but if `operandType` is an interface, type, use it but defaults-to-bounds args.
i.e. re-write `operandType` to a safe version
So perhaps the more interesting question: we have a failing test, but getting the test passing is insufficient to have correct behavior?
How do we define, in a test, what is sufficient testing for correct behavior?
Related details
- Jackson Gardner
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
: operandType,Martin KustermannEven though it is "safe" to do that, I don't think it's the right fix.
It completely ignores `operandType` and thereby ignores valuable information that makes the checks cheaper.
That can be locally fixed here by making use of parts of `operandType` that we know are safe to use and discarding other parts.
Though after thinking a bit more about this, I come to the conclusion that this is a symptom of a bigger issue and it would be best handled in the CFE instead.
I've talked to the CFE team and they seem to agree and will look into fixing it in a better way.
Kevin MooreIf you really want to land it before handling this in the CFE, you can change this to use nullable/non-nullable top type in general (we can rely on the nullability), but if `operandType` is an interface, type, use it but defaults-to-bounds args.
i.e. re-write `operandType` to a safe version
So perhaps the more interesting question: we have a failing test, but getting the test passing is insufficient to have correct behavior?
How do we define, in a test, what is sufficient testing for correct behavior?
Passing tests doesn't imply correct behavior. A test can only prove presence of bugs not the absence of all bugs.
This particular code your agent modified is a performance optimization. The way we would test performance optimizations is using IR tests.
Would you be willing to update `operandType` as suggested above to make us continue to optimize the checks but to so safely?
Related details
- Jackson Gardner
- Kevin Moore
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Moore (Gerrit)
Kevin Moore voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kevin Moore
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
Martin KustermannEven though it is "safe" to do that, I don't think it's the right fix.
It completely ignores `operandType` and thereby ignores valuable information that makes the checks cheaper.
That can be locally fixed here by making use of parts of `operandType` that we know are safe to use and discarding other parts.
Though after thinking a bit more about this, I come to the conclusion that this is a symptom of a bigger issue and it would be best handled in the CFE instead.
I've talked to the CFE team and they seem to agree and will look into fixing it in a better way.
Kevin MooreIf you really want to land it before handling this in the CFE, you can change this to use nullable/non-nullable top type in general (we can rely on the nullability), but if `operandType` is an interface, type, use it but defaults-to-bounds args.
i.e. re-write `operandType` to a safe version
Martin KustermannSo perhaps the more interesting question: we have a failing test, but getting the test passing is insufficient to have correct behavior?
How do we define, in a test, what is sufficient testing for correct behavior?
Passing tests doesn't imply correct behavior. A test can only prove presence of bugs not the absence of all bugs.
This particular code your agent modified is a performance optimization. The way we would test performance optimizations is using IR tests.
Would you be willing to update `operandType` as suggested above to make us continue to optimize the checks but to so safely?
I added a test to validate the new behavior. Tests can't catch all bugs, but they can ensure we don't regress! 😊
Related details
- Jackson Gardner
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 4 comments![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
4 comments
This change treats the static operand type as Object? during the typeThis isn't true anymore
/// violate soundness.because the runtime type might be a subtype
Did AI write this?
If "the runtime type might be a subtype" were true, then we wouldn't have a problem, because the static type would be correct.
The issue here is that the static type of the `<operand>` in `<operand> as <testedAgainstType>` is incorrect, meaning the runtime value isn't a subtype of `<operand>`s static type.
DartType _safeCovarianceOperandType(DartType operandType) {nit: Consider making code be readable from top to bottom in most cases (so it reads like a book): i.e. move this helper below `emitAsCheck`
isCovarianceCheck ? _safeCovarianceOperandType(operandType) : operandType,Can we instead have
```
operandType = isCovarianceCheck
? _safeCovarianceOperandType(operandType)
: operandType;
```
as the first thing in the function?
Then all of the code deals with a safe `operandType`, including `checkOnlyNullAssignability`, which can be simplified then
Related details
- Jackson Gardner
- Kevin Moore
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |