Fix SubText ctor parameter verification. #78989
Conversation
This ctor currently throws given a zero length span at the end (and only the end) of the subtext. This evidently became far more prevalent with this (fairly) recent change to optimize newline information in our sourcetext classes. (dotnet#74728) It doesn't make to allow zero length spans at all locations but at the end of the subtext, so the fix is just to allow construction in that case too. Big props to Manish Vasani for providing a very actionable repro for this. Fixes dotnet#78985. Note there is a potentially related issue outlined in dotnet#76225 that looks attributable to the PR above too. It possible that this addresses that issue too, but I'm not completely convinced.
|
Is this the cause of the other issues we're seeing getting token spans? |
| } | ||
|
|
||
| // span.Start and span.End are valid wrt eachother by nature of being passed in as a TextSpan, | ||
| // so there is no need to verify span.Start against text.Length or span.End against zero. |
There was a problem hiding this comment.
I don't follow how this is true. I can absolutely construct a bad TextSpan and pass it in, what prevents me from doing that?
There was a problem hiding this comment.
a textspan itself prevents the end from being before the start. so a text span's start/end always represent at least a legal range where end >= start. given that. you only ened to validate that the start of this text span is not before the start of the text, and the end of the text span is not after the end of the text.
There was a problem hiding this comment.
In that case I might need to change this. I'm curious as to what mechanism would allow creation of the object such that the TextSpan ctor lets the object creation proceed while violating these constraints. Is it through serialization which sets the backing Start/Length properties through reflection?
*** Edit: this comment was made before I saw Cyrus's response
|
@jaredpar i think we should consider this for servicing. we've def been getting a range of random reports about random breakage in source-texts. If the fix is really this trivial, then it would be good to be able to backport. |
I'm unsure. The callstack from the other bug doesn't quite match this, so it seems like a different issue. However that PR is likely the culprit of the other issue and it appears to be somehow surfaced around mismatching newline types, which is how this issue was found too. |
Can you help me by getting a list of bugs / reports that demonstrate impact? That is the key for me getting issues through QB mode. |
|
@ToddGrun ^ ? |
The related bugs would be #76225 and #78639. I have pretty low confidence this is the same issue though. We did add dump reporting for those issues and backported to 17.14 (#78367), but my searches aren't finding the dmps that this should be generating. @jasonmalinowski -- Can you help me locate a dmp for the FatalError.ReportWithDumpAndCatch call that was added in 17.14? |
Fix more Update versions Fix more In progress Flip Fix Fix remove wrapper Fix Fix in progress Fix DeclarationKind layering fix One building In progress Breaking out refactoring helpers into core api vs service VB side In progress Move type workaround Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CSharpCompilerExtensions.projitems Fix SubText ctor parameter verification. (dotnet#78989) * Fix SubText ctor parameter verification. This ctor currently throws given a zero length span at the end (and only the end) of the subtext. This evidently became far more prevalent with this (fairly) recent change to optimize newline information in our sourcetext classes. (dotnet#74728) It doesn't make to allow zero length spans at all locations but at the end of the subtext, so the fix is just to allow construction in that case too. Big props to Manish Vasani for providing a very actionable repro for this. Fixes dotnet#78985. Note there is a potentially related issue outlined in dotnet#76225 that looks attributable to the PR above too. It possible that this addresses that issue too, but I'm not completely convinced. Extensions: bind unconditionally of LangVer (dotnet#78947) Extensions: use specific tracking issues for different areas (second pass) (dotnet#78971) Fix nullable analysis involving extensions and collection expressions (dotnet#78926) Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CSharpCompilerExtensions.projitems in progrss Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CompilerExtensions.projitems Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/VisualBasicCompilerExtensions.projitems extensions Fix RootNamespace Fixes Fixes IN progress in progress
This reverts commit 97445ba.
6 participants
This ctor currently throws given a zero length span at the end (and only the end) of the subtext. This evidently became far more prevalent with this (fairly) recent change to optimize newline information in our sourcetext classes. (#74728)
It seens odd to allow zero length spans at all locations but at the end of the subtext, so the fix is to allow construction in that case too.
Big props to Manish Vasani for providing a very actionable repro for this.
Fixes #78985
Note there is a potentially related issue outlined in #76225 that looks attributable to the PR above too. It is possible that this addresses that issue too, but I'm not completely convinced.