Conversation
|
Looking into the test failure. Didn't come up for because its not marked with the same trait. |
|
I've fixed the test failure, and re-run all of the related tests (I hope) however I'd love to get some feedback on the change. I'm wondering if checking the validity of the conversion is the right way to go, or if there might be other edge cases this breaks, but that there aren't tests for. I couldn't find anything readily accessible that pointed in the direction of trying to identify ref structs, but also its not clear that casting from object to a ref struct is worthy enough for a special case for this diagnostic. |
|
What exactly was the test failure? Is it possible there was an accidental bug in the test? I don't really understand the fix. |
|
If the test contains incorrect code, it's probably a good thing that we don't offer to simplify. |
|
I think the test failure was genuine. The failure can be seen here The test in question, |
|
Having said that, the test that failed does have invalid code, in that there is a compile error, however the diagnostic in this case actually corrects the code, so I'm assuming its a desired outcome to have it. |
|
Well, the cast is indeed invalid in that case (parameter type doesn't match), so this certainly feels similar to the original issue. The fact that by removing the cast, the code becomes valid is sort of a coincidence. |
|
Unless there is some way to track down why that test was added, it's not really possible to know if it was coincidence or design. I can see arguments either way. You wouldn't want annoying suggestions possible on code that doesn't compile, but at the same time we want the editor to help us while we're coding, not just once we've finished. |
| { | ||
| var castToOuterType = semanticModel.ClassifyConversion(cast.SpanStart, cast, outerType); | ||
| var expressionToOuterType = GetSpeculatedExpressionToOuterTypeConversion(speculationAnalyzer.ReplacedExpression, speculationAnalyzer, cancellationToken); | ||
| var originalIsAnonymousFunction = expressionToOuterType.IsAnonymousFunction; |
There was a problem hiding this comment.
I actually dislike introducing a local variable here because it hurts readability later.
| expressionToCastType == expressionToOuterType) | ||
| expressionToCastType == expressionToOuterType && | ||
| // if the conversion to the outer type doesn't exist, then we shouldn't offer, except for anonymous functions which can't be reasoned about | ||
| (expressionToOuterType.Exists || originalIsAnonymousFunction)) |
There was a problem hiding this comment.
Here I'm left wondering what "original" is -- is that expressionToCastType or expressionToOuterType. I'd rather just see the access to expressionToOuterType.IsAnonymousFunction here. That's already very clear and consistent with the rest of the method.
There was a problem hiding this comment.
The problem is that expressionToOuterType is reassigned on line 461 so I needed to capture the state of the previous value before that reassignment.
Calling GetSpeculatedExpressionToOuterTypeConversion again would be too costly I'm assuming. Would a rename of the local help? I could try putting in an earlier exit for this case and see if it breaks any other tests.
There was a problem hiding this comment.
Ah! That makes sense. In that case, there are a couple of options:
- Rename the local to better describe what is expected in this condition. This might get long. E.g.
originallySpeculatedExpressionToOuterTypeWasAnonymousFunctionConversion-- though I'm not sure that's not quite right. 😄 - Change the logic that reassigns
expressionToOuterTypeto capture the original conversion. I could image something like this:
var speculativeExpressionToOuterType = GetSpeculatedExpressionToOuterTypeConversion(speculationAnalyzer.ReplacedExpression, speculationAnalyzer, cancellationToken);
// CONSIDER: Anonymous function conversions cannot be compared from different semantic models as lambda symbol comparison requires syntax tree equality. Should this be a compiler bug?
// For now, just revert back to computing expressionToOuterType using the original semantic model.
var expressionToOuterType = speculativeExpressionToOuterType.IsAnonymousFunction
? semanticModel.ClassifyConversion(cast.SpanStart, cast.Expression, outerType)
: speculativeExpressionToOuterType;Then, you could test speculativeExpressionToOuterType.IsAnonymousFunction here.
DustinCampbell
left a comment
There was a problem hiding this comment.
An even better solution. Nice!
|
Approved to merge for 15.8.PReview4 |
|
Thank you, @davidwengier, for your help! |
|
No thank you, and the rest of Microsoft, for making it possible for me to work on Roslyn, and have a conversation with some of its key team members. Hopefully another bigger, more interesting PR coming soon :) |
Don't offer unnecessary cast fixer on invalid code
Customer scenario
When trying to cast to a TypedReference (or presumably any other ref-like struct) from object, the RemoveUnnecessaryCast diagnostic pops up which is confusing, since its incorrect, and can hide the underlying compilation error. Allowing the error to bubble up in this case is a better user experience.
Bugs this fixes
Fixes: #27239
Workarounds, if any
None really.
Risk
There is a small chance that there could be scenarios where the diagnostic is desired even though no conversion is valid, but I couldn't think of one. An alternate approach if necessary could be to make the diagnostic use similar logic to the compiler in determining specifically that the unboxing conversion will be going to something stack only, and not show in those circumstances.
Performance impact
Low, because the analyzer had already done the work
Is this a regression from a previous update?
Not as far as I know
Root cause analysis
I added a test for this case. Presumably missed because most (all?) other invalid conversions meet one of the other conditions, ie explicit, implicit, unboxing etc.
How was the bug found?
Not much detail on the issue. I just picked something off the "Help Wanted" label 🙂
Test documentation updated?
N/A