-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
'Microsoft.CodeAnalysis.CSharp.Diagnostics.AddBraces.CSharpAddBracesDiagnosticAnalyzer' threw an exception of type 'System.InvalidOperationException' with message 'Unexpected true' #33560
Comments
Reported at DC (link) Priority 1 |
Hmm, I couldn't trigger the error with code provided (and setting FYI @sharwell who might have some insight here |
I don't repro with VS 2019 preview 5. So, maybe it's already fixed... |
Closed sicne we are unable to repro this issue. Please reopen and provide us more information on how to repro if you are still seeing it. |
I see that this was closed; however I am receiving the same errors on 16.0.4. On a large solution, I am receiving ~150 warnings with this error. It seems to be occurring around nested if statements where the deeper if/else is single line. Thanks, |
I created a pull request that addresses the issue. The resolution should be reviewed, but the issue was traced: The |
I'm getting the same issue in private void Advance() {
try {
int next = input.Read();
if (next >= 0)
currentChar = (char)next;
else
currentChar = '\0';
} catch (IOException) {
currentChar = '\0';
}
} public Label DefaultTarget() {
if (targets.Count > 0)
return targets[targets.Count - 1];
else
return null;
} |
@effleurager Thanks for reporting the issue! I have tried the following code using the version you specified as well as the latest in master, but still couldn't repro the crash :( What's the codestyle preference you set for braces (I did try all three options though)? Are you able to hit the crash with the code below? class Program
{
string[] targets;
private Program input;
private char currentChar;
public string DefaultTarget()
{
if (targets.Count() > 0)
return targets[targets.Count() - 1];
else
return null;
}
public void Advance()
{
try {
int next = input.Read();
if (next >= 0)
currentChar = (char)next;
else
currentChar = '\0';
} catch (IOException) {
currentChar = '\0';
}
}
char Read() => 'a';
} |
I have been looking into this issue, I can reproduce the exception while debugging the unit tests by manually bypassing a certain check, and from my testing a simple fix does solve the issue. That said I am unable to understand why/how a SyntaxToken reaches the symptomatic state. Within the In normal operation, this method uses the SyntaxTree's text to verify if the two tokens are on the same line, in which case checking the same token is unnecessary, but works fine. But (here is the state I don't understand) if the token's SyntaxTree is null or the TryGetText method fails, then an alternate methodology is used which contractually throws an exception when the tokens are the same. I assume there must be a condition or state where a token's SyntaxTree is null or the TryGetText method will fail, otherwise this code would not have been written - but I do not know how to reproduce this state. All of the relevant code: From ...
// Check the part of the statement preceding the embedded statement (bullet 1)
var lastTokenBeforeEmbeddedStatement = embeddedStatement.GetFirstToken().GetPreviousToken();
if (!FormattingRangeHelper.AreTwoTokensOnSameLine(statement.GetFirstToken(), lastTokenBeforeEmbeddedStatement))
{
// The part of the statement preceding the embedded statement does not fit on one line. Examples:
//
// for (int i = 0; // <-- The initializer/condition/increment are on separate lines
// i < 10;
// i++)
// SomeMethod();
return true;
}
... With a statement like The main code in question is in public static bool AreTwoTokensOnSameLine(SyntaxToken token1, SyntaxToken token2)
{
var tree = token1.SyntaxTree;
if (tree != null && tree.TryGetText(out var text)) // <-- When / how does this fail?
{
return text.AreOnSameLine(token1, token2);
}
// This call eventually throws if token1 == token2
return !CommonFormattingHelpers.GetTextBetween(token1, token2).ContainsLineBreak();
} How do we get a token with no SyntaxTree or failed TryGetText call? Lastly, the simple fix is to update the public static bool AreTwoTokensOnSameLine(SyntaxToken token1, SyntaxToken token2)
{
if(token1 == token2) return true;
... |
@genlu This is my .editorconfig file, but it seems that the issue is rather temperamental as I cannot reproduce the issue on the same initial code or your boiled down class, despite running the same version. |
Yeah, that's what's been puzzling me. There's seems to be some nondeterminism of this bug. @Andrew-Hanlon Thanks for the detailed analysis! @CyrusNajmabadi do you know in what scenario we might get null from |
Haven't read everything. But is the token a |
I'm not sure why we woudl think this would every necessarily succeed. i.e. just because you have the tree, the text may still be dumped from memory. For example, if this was just some forked tree or analyzing some old snapshot that the main VS buffer had moved forward from. -- @jasonmalinowski What's our rules/invariants around TryGetText? I don't think we have a guarantee around it that would make it work for something like this. |
@CyrusNajmabadi Thank you, that's good to know. Would you know if there is a way to set-up a unit test for such a scenario (dumped from memory for example)? In any case, I know my suggested fix solves the issue. I closed a previous PR as I could not produce a unit test to capture the scenario, but I can open a new one with some of these details. |
That's a great question. I imagine (and maybe @jasonmalinowski could help here), but you should be able to just work with teh workspace, get the snapshot for the existing buffer, then actually edit/close the buffer, then GC a lot... maybe you'd see it happen? I'm not really sure where we ever landed in terms of hte invariants around |
A little bit more debugging information: I was able to debug this situation by running the Roslyn solution and attaching the debugger to the When the 'error' state was hit, I was able to see in the immediate window that indeed it is the The stacktrace is below:
Likewise by applying the equality check fix above, this exception is no longer hit. |
Without looking at the code, the only rule I'd be comfortable saying about TryGetText is you shouldn't use it. 😄 Now to back to trying to be useful.
I don't think there are formally documented rules. The method is only intended for optimizations where you can potentially fast-path something. I suspect you're right that this could fail if you have somebody rewriting a tree, and we haven't yet computed text, or if we did were only holding text lazily. Even if there are "rules" I assume things would get complicated: I would assume that a syntax tree that didn't have realized text would fail TryGetText, but once somebody called GetText() (which is possibly a different extension in the same process!) then maybe it'd work. If we wanted a unit test to cover a case I wouldn't want the unit test trying to reproduce a "conditions where we know this will fail" because the test is tied to an implementation detail in a different system. I'd say either have the test create it's own derived version of SyntaxTree that will fail TryGetText, or just test the underlying methods there that they behave the same and trust you've got both paths right. |
Yeha, the only rule i've ever had is: this is safe if you are certain someone higher in your calltree grabbed this thing and put it in a local** -- ** And even then, you probably need a GC.KeepAlive. And even then... why are you not just passing this value downstream to the people who clearly need it? |
Thanks for the insight all. So my proposal is that I will create a PR that includes the fix plus a new unit test that covers the |
Fix #33560 Handle equal tokens where Text is not available.
Version Used:
VS 2019 preview 3
The code around the error (The line comments are not in the original code)
The text was updated successfully, but these errors were encountered: