Skip to content

Do not attempt to move diagnostics with additional locations. - #10302

Merged
CyrusNajmabadi merged 3 commits into
dotnet:masterfrom
CyrusNajmabadi:multipleInvokeDelegate
Apr 5, 2016
Merged

CyrusNajmabadi merged 3 commits into
dotnet:masterfrom
CyrusNajmabadi:multipleInvokeDelegate

Conversation

@CyrusNajmabadi

Copy link
Copy Markdown
Contributor

Fixes #10258

The issue here is that the "InvokeDelegateWithConditionalAccess" analyzer uses 'additional locations' on diagnostic datas to pass along data. Unfortunately, the diagnostic analyzer infrastructure was not properly updating these spans when it attempted to preserve and move around diagnostics.

Updating these diagnostics is tricky to do properly. Additional locations can span across documents or may be in arbitrary locations in the same docoument as the diagnostic. As such, if have those diagnostics, we don't try to reuse and move the owning diagnostics. Instead, we reacquire these diagnostics so that all the data in them is correct.

Because of the complexity of this repro the testing will be done with an integration test in roslyn-internal.

return diagnostic.AdditionalLocations == null || diagnostic.AdditionalLocations.Count == 0;
}

private static async Task<ImmutableArray<DiagnosticData>> UpdateAllSemanticDiagnostics(StateSet stateSet, DiagnosticAnalyzerDriver analyzerDriver, ImmutableArray<DiagnosticData> diagnosticData)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suffix with Async?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

@mavasani

mavasani commented Apr 3, 2016

Copy link
Copy Markdown
Contributor

Tagging @heejaechang so the v2 OOP engine also handles this.

@CyrusNajmabadi

Copy link
Copy Markdown
Contributor Author

Ping @dotnet/roslyn-ide

@jmarolf

jmarolf commented Apr 4, 2016

Copy link
Copy Markdown
Contributor

Can you point us at the PR that contains the internal tests?

@CyrusNajmabadi

Copy link
Copy Markdown
Contributor Author

@CyrusNajmabadi

Copy link
Copy Markdown
Contributor Author

test vsi please

@jmarolf

jmarolf commented Apr 5, 2016

Copy link
Copy Markdown
Contributor

👍


diagnosticData = _owner.UpdateDocumentDiagnostics(existingData, ranges.Ranges, memberDxData.AsImmutableOrEmpty(), root.SyntaxTree, member, memberId);
ValidateMemberDiagnostics(stateSet.Analyzer, document, root, diagnosticData);
// if all the current diagnostics have all their locations in this document,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this method shouldn't be called for analyzer that can't do member update. just make the analyzer never to be in this method.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm.. this code seems changed quite a bit over what I remember. anyway, v2 engine doesn't make span based analysis anymore so it shouldn't be an issue for v2.

@heejaechang

Copy link
Copy Markdown
Contributor

👍

@CyrusNajmabadi

Copy link
Copy Markdown
Contributor Author

retest vsi please

@CyrusNajmabadi
CyrusNajmabadi merged commit 51edce0 into dotnet:master Apr 5, 2016
@CyrusNajmabadi
CyrusNajmabadi deleted the multipleInvokeDelegate branch April 5, 2016 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants