Skip to content

C#: Workaround Roslyn bug in INamedTypeSymbol.TupleElements #7646

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 19, 2022
Merged

C#: Workaround Roslyn bug in INamedTypeSymbol.TupleElements #7646

merged 2 commits into from
Jan 19, 2022

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Jan 19, 2022

We occasionally see errors like this:

Unhandled exception generating id: Object reference not set to an instance of an object. in Label.ToString()    at Semmle.Extraction.CSharp.SymbolExtensions.<>c__DisplayClass11_0.<BuildNamedTypeId>b__0(IFieldSymbol f) in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/SymbolExtensions.cs:line 293
   at Semmle.Extraction.TrapExtensions.BuildList[T1,T2](T1 trapFile, String separator, IEnumerable`1 items, Action`1 action) in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/TrapExtensions.cs:line 232
   at Semmle.Extraction.CSharp.SymbolExtensions.BuildNamedTypeId(INamedTypeSymbol named, Context cx, EscapingTextWriter trapFile, ISymbol symbolBeingDefined, Boolean constructUnderlyingTupleType) in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/SymbolExtensions.cs:line 340
   at Semmle.Extraction.CSharp.SymbolExtensions.BuildTypeId(ITypeSymbol type, Context cx, EscapingTextWriter trapFile, ISymbol symbolBeingDefined, Boolean constructUnderlyingTupleType) in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/SymbolExtensions.cs:line 203
   at Semmle.Extraction.CSharp.Entities.TupleType.WriteId(EscapingTextWriter trapFile) in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/TupleType.cs:line 34
   at Semmle.Extraction.Entity.WriteQuotedId(EscapingTextWriter trapFile) in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Entities/Base/Entity.cs:line 25
   at Semmle.Extraction.Entity.DefineLabel(TextWriter trapFile, Extractor extractor) in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Entities/Base/Entity.cs:line 47

and

Uncaught exception. Object reference not set to an instance of an object.    at Microsoft.CodeAnalysis.CSharp.SymbolDisplayVisitor.AddTupleTypeName(INamedTypeSymbol symbol)
   at Microsoft.CodeAnalysis.CSharp.SymbolDisplayVisitor.AddNameAndTypeArgumentsOrParameters(INamedTypeSymbol symbol)
   at Microsoft.CodeAnalysis.CSharp.SymbolDisplayVisitor.MinimallyQualify(INamedTypeSymbol symbol)
   at Microsoft.CodeAnalysis.CSharp.SymbolDisplayVisitor.VisitNamedTypeWithoutNullability(INamedTypeSymbol symbol)
   at Microsoft.CodeAnalysis.CSharp.SymbolDisplayVisitor.VisitNamedType(INamedTypeSymbol symbol)
   at Microsoft.CodeAnalysis.CSharp.Symbols.PublicModel.NamedTypeSymbol.Accept(SymbolVisitor visitor)
   at Microsoft.CodeAnalysis.CSharp.Symbols.PublicModel.Symbol.Microsoft.CodeAnalysis.ISymbol.Accept(SymbolVisitor visitor)
   at Microsoft.CodeAnalysis.CSharp.SymbolDisplay.ToDisplayParts(ISymbol symbol, SemanticModel semanticModelOpt, Int32 positionOpt, SymbolDisplayFormat format, Boolean minimal)
   at Microsoft.CodeAnalysis.CSharp.SymbolDisplay.ToDisplayParts(ISymbol symbol, SymbolDisplayFormat format)
   at Microsoft.CodeAnalysis.CSharp.SymbolDisplay.ToDisplayString(ISymbol symbol, SymbolDisplayFormat format)
   at Microsoft.CodeAnalysis.CSharp.Symbols.PublicModel.Symbol.ToString()
   at Semmle.Extraction.Message.Create(Context cx, String text, ISymbol symbol, String stackTrace, Severity severity) in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Message.cs:line 31
   at Semmle.Extraction.Context.Try(SyntaxNode node, ISymbol symbol, Action a) in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Context.cs:line 543
   at Semmle.Extraction.Context.<>c__DisplayClass41_0.<Populate>b__2() in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Context.cs:line 385
   at Semmle.Extraction.Context.Populate(ISymbol optionalSymbol, CachedEntity entity) in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Context.cs:line 390
   at Semmle.Extraction.Context.<>c__DisplayClass41_0.<Populate>b__0() in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Context.cs:line 343
   at Semmle.Extraction.ContextShared.WithWriter(TextWriter trapFile, Action a) in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/ContextShared.cs:line 133
   at Semmle.Extraction.Context.<>c__DisplayClass28_0.<PopulateLater>b__1() in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Context.cs:line 226
   at Semmle.Extraction.Context.PopulateAll() in /home/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Context.cs:line 245

It appears to be the same issue as discussed in dotnet/roslyn#53943.

@hvitved hvitved requested a review from a team as a code owner January 19, 2022 10:39
@github-actions github-actions bot added the C# label Jan 19, 2022
tamasvajk
tamasvajk reviewed Jan 19, 2022
View reviewed changes
tamasvajk
tamasvajk previously approved these changes Jan 19, 2022
View reviewed changes
@hvitved
Copy link
Contributor Author

hvitved commented Jan 19, 2022

First approach did not work, since it will potentially produce overlapping TRAP IDs, and in effect DB inconsistencies like

[INVALID_KEY] predicate tuple_underlying_type(@tuple_type tuple, @type_or_ref struct): The key set {tuple} does not functionally determine all fields.
Here is a pair of tuples that agree on the key set but differ at index 1:
Tuple 1 in row 1: (1288,597)
Tuple 2 in row 2: (1288,111668)
	Relevant element: Tuple 1: tuple=1288
		Full ID for 1288: @"();tuple"
	Relevant element: Tuple 2: tuple=1288
		Full ID for 1288: @"();tuple"
	Relevant element: Tuple 1: struct=597
		Full ID for 597: @"(17).ValueTuple`2;type". The ID may expand to @"{@"{@";namespace"}.System;namespace"}.ValueTuple`2;type"
	Relevant element: Tuple 2: struct=111668
		Full ID for 111668: @"(17).ValueTuple;type". The ID may expand to @"{@"{@";namespace"}.System;namespace"}.ValueTuple;type"

@tamasvajk
Copy link
Contributor

First approach did not work, since it will potentially produce overlapping TRAP IDs, and in effect DB inconsistencies like

Good catch.

@hvitved
Copy link
Contributor Author

hvitved commented Jan 19, 2022

Good catch.

Yeah, good catch CI ;-)

tamasvajk
tamasvajk reviewed Jan 19, 2022
View reviewed changes
csharp/extractor/Semmle.Extraction.CSharp/Entities/TypeMention.cs
@@ -90,7 +90,11 @@ protected override void Populate(TextWriter trapFile)
var tts = (TupleTypeSyntax)syntax;
var tt = (TupleType)type;
Emit(trapFile, loc ?? syntax.GetLocation(), parent, type);
tts.Elements.Zip(tt.TupleElements, (s, t) => Create(Context, s.Type, this, t.Type)).Enumerate();
foreach (var (s, t) in tts.Elements.Zip(tt.TupleElements, (s, t) => (s, t?.Type)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we be more explicit, and use GetTupleElementsMaybeNull() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely. I was sure I had done that, but I guess not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, wait, that is actually the field Field?[] TupleElements on the class TupleType, which uses GetTupleElementsMaybeNull internally.

@hvitved hvitved merged commit 70f4efb into github:main Jan 19, 2022
@hvitved hvitved deleted the csharp/roslyn-tuple-elements-workaround branch January 19, 2022 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tamasvajk tamasvajk tamasvajk approved these changes

Assignees

@tamasvajk tamasvajk

Labels
C#
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hvitved @tamasvajk