C#: Fixes for AST printing #4438
Conversation
e24d506 to
4e4defa
Compare
4e4defa to
59ca516
Compare
I'm surprised to see the changes in the diffs. None of them seem to be attribute and namespace declaration related though. |
8400048 to
af47495
Compare
756707f to
abff3af
Compare
| # 4| 0: [LocalVariableDeclAndInitExpr] Int32[] one_dim = ... | ||
| # 4| -1: [TypeMention] Int32[] | ||
| # 4| -1: [TypeMention] int[] | ||
| # 4| 1: [TypeMention] int |
There was a problem hiding this comment.
@hvitved The main motivation for this commit was to unify the type name printing of TypeMentions. I think this looks a bit better, but we're still showing Int32[] in the above LocalVariableDeclAndInitExpr (and in some other nodes too). Should/Can I change those classes to use the Type.toStringWithTypes? It feels a bit strange to change all these base class toString()s just for the sake of AST printing.
What was the initial idea behind toString vs toStringWithTypes?
| # 30| -1: [TypeMention] Nullable[] | ||
| # 30| 1: [TypeMention] Nullable<Byte> | ||
| # 30| 1: [TypeMention] Byte | ||
| # 30| -1: [TypeMention] Nullable<Byte>[][] |
There was a problem hiding this comment.
Nullable[] vs Nullable<Byte>[][] is the reason why toStringWithTypes() was used.
909e12b to
cba8e28
Compare
cba8e28 to
74fa7e4
Compare
|
The differences job revealed a couple of type mention extraction failures... |
| # 131| 16: [Field] l | ||
| # 131| -1: [TypeMention] MyClass[,,][,,,][][,] | ||
| # 131| 1: [TypeMention] MyClass | ||
| # 131| 1: [TypeMention] MyClass | ||
| # 131| 1: [TypeMention] MyClass | ||
| # 131| 1: [TypeMention] MyClass |
There was a problem hiding this comment.
This corresponds to MyClass?[,,,][][,]?[,,] l;.
The type mention hierarchy is now similar to MyClass[,,,][][,][,,] k; a couple of lines above, that is, there's only one type mention for the multi dimensional array and one for the element type.
9feaf19 to
5d0c30d
Compare
|
Note to self: add change notes. |
csharp/extractor/Semmle.Extraction.CSharp/Entities/NamespaceDeclaration.cs
Outdated
Show resolved
Hide resolved
csharp/extractor/Semmle.Extraction.CSharp/Entities/NamespaceDeclaration.cs
Outdated
Show resolved
Hide resolved
| @@ -23,27 +23,36 @@ private TypeMention(Context cx, TypeSyntax syntax, IEntity parent, Type type, Mi | |||
| this.loc = loc; | |||
| } | |||
|
|
|||
| private static TypeSyntax GetElementType(TypeSyntax type) | |||
| private TypeSyntax GetArrayElementType(TypeSyntax type) | |||
There was a problem hiding this comment.
Perhaps GetUnderlyingType is a better name?
There was a problem hiding this comment.
I thought about this too. This method should not be called from case SyntaxKind.NullableType in Populate. It should only be used to get the element type of array-like TypeSyntax elements (array, pointer), hence the name. However, it's recursive and during the recursion, we need to also handle NullableTypeSyntax, and this introduces a bit of confusion.
| | definitions.cs:121:18:121:18 | A | definitions.cs:97:11:97:11 | A | T | | ||
| | definitions.cs:121:21:121:22 | I1 | definitions.cs:112:15:112:16 | I1 | T | | ||
| | definitions.cs:121:25:121:26 | I2<A> | definitions.cs:117:15:117:19 | I2<> | T | | ||
| | definitions.cs:121:25:121:29 | I2<A> | definitions.cs:117:15:117:19 | I2<> | T | |
There was a problem hiding this comment.
This change is problematic, as we will now have overlapping location spans for the character A: one for I2<A> (which jumps to I2<>) and one for A (which jumps to A), so not sure if the IDE/LGTM will work correctly when trying to jump to the definition for A. I agree that the right location for I2<A> is the whole span, so perhaps the fix instead is to adjust hasLocationInfo for constructed types to exclude type arguments, just like we remove qualifiers from method calls.
There was a problem hiding this comment.
Thanks for noticing. I pushed a fix for this.
| TTopLevelTypeMentionNode(TypeMention typeMention) { | ||
| shouldPrint(typeMention.getTarget(), _) and | ||
| not isNotNeeded(typeMention.getTarget().getParent*()) and | ||
| not typeMention.getTarget().getParent*() instanceof TypeParameter | ||
| } or | ||
| TNestedTypeMentionNode(TypeMention typeMention) { | ||
| shouldPrint(typeMention.getParent+().getTarget(), _) and | ||
| not isNotNeeded(typeMention.getParent+().getTarget().getParent*()) and | ||
| not typeMention.getParent+().getTarget().getParent*() instanceof TypeParameter | ||
| } or |
There was a problem hiding this comment.
Is there a good reason to distinguish nested and top-level type mentions? It seems that something like this would encompass both cases:
TTypeMentionNode(TypeMention typeMention) {
exists(Element target |
target = typeMention.getParent*().getTarget() and
shouldPrint(target, _) and
not isNotNeeded(target.getParent*()) and
not target.getParent*() instanceof TypeParameter
)
}There was a problem hiding this comment.
We need to differentiate between the two, because we only want to have top level type mentions directly under ElementNodes and BaseTypesNodes.
There was a problem hiding this comment.
But that could be handled by restricting to those TypeMentionNodes for which getTarget() exists.
There was a problem hiding this comment.
Makes sense. Let me adjust it.
No description provided.