C#: Address review comments.

calumgrant · calumgrant · commit 0129b42c5420 · 2019-08-30T10:11:01.000+01:00
diff --git a/csharp/extractor/Semmle.Extraction.CIL/Entities/Method.cs b/csharp/extractor/Semmle.Extraction.CIL/Entities/Method.cs
@@ -43,11 +43,11 @@ protected Method(GenericContext gc) : base(gc.cx)
         public virtual IList<LocalVariable> LocalVariables => throw new NotImplementedException();
         public IList<Parameter> Parameters { get; private set; }
 
-        public override void WriteId(TextWriter trapFile) => MakeMethodId(trapFile, DeclaringType, NameLabel);
+        public override void WriteId(TextWriter trapFile) => WriteMethodId(trapFile, DeclaringType, NameLabel);
 
         public abstract string NameLabel { get; }
 
-        internal protected void MakeMethodId(TextWriter trapFile, Type parent, string methodName)
+        internal protected void WriteMethodId(TextWriter trapFile, Type parent, string methodName)
         {
             signature.ReturnType.WriteId(trapFile, this);
             trapFile.Write(' ');
diff --git a/csharp/extractor/Semmle.Extraction.CIL/Entities/Type.cs b/csharp/extractor/Semmle.Extraction.CIL/Entities/Type.cs
@@ -131,12 +131,12 @@ public virtual IEnumerable<Type> ThisTypeArguments
         }
 
         /// <summary>
-        /// Gets the assembly identifier of this type.
+        /// Writes the assembly identifier of this type.
         /// </summary>
         public abstract void WriteAssemblyPrefix(TextWriter trapFile);
 
         /// <summary>
-        /// Gets the ID part to be used in a method.
+        /// Writes the ID part to be used in a method ID.
         /// </summary>
         /// <param name="inContext">
         /// Whether we should output the context prefix of type parameters.
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Accessor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Accessor.cs
@@ -34,8 +34,8 @@ IPropertySymbol PropertySymbol
         public override void Populate(TextWriter trapFile)
         {
             PopulateMethod(trapFile);
-            ExtractModifiers(trapFile);
-            ContainingType.ExtractGenerics();
+            PopulateModifiers(trapFile);
+            ContainingType.PopulateGenerics();
 
             var prop = PropertySymbol;
             if (prop == null)
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs
@@ -17,8 +17,8 @@ public class Constructor : Method
         public override void Populate(TextWriter trapFile)
         {
             PopulateMethod(trapFile);
-            ExtractModifiers(trapFile);
-            ContainingType.ExtractGenerics();
+            PopulateModifiers(trapFile);
+            ContainingType.PopulateGenerics();
 
             trapFile.constructors(this, symbol.ContainingType.Name, ContainingType, (Constructor)OriginalDefinition);
             trapFile.constructor_location(this, Location);
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Destructor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Destructor.cs
@@ -11,8 +11,8 @@ class Destructor : Method
         public override void Populate(TextWriter trapFile)
         {
             PopulateMethod(trapFile);
-            ExtractModifiers(trapFile);
-            ContainingType.ExtractGenerics();
+            PopulateModifiers(trapFile);
+            ContainingType.PopulateGenerics();
 
             trapFile.destructors(this, string.Format("~{0}", symbol.ContainingType.Name), ContainingType, OriginalDefinition(Context, this, symbol));
             trapFile.destructor_location(this, Location);
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Event.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Event.cs
@@ -21,7 +21,7 @@ public override void WriteId(TextWriter trapFile)
 
         public override void Populate(TextWriter trapFile)
         {
-            ExtractNullability(trapFile, symbol.NullableAnnotation);
+            PopulateNullability(trapFile, symbol.NullableAnnotation);
 
             var type = Type.Create(Context, symbol.Type);
             trapFile.events(this, symbol.GetName(), ContainingType, type.TypeRef, Create(Context, symbol.OriginalDefinition));
@@ -35,7 +35,7 @@ public override void Populate(TextWriter trapFile)
             if (!(remover is null))
                 Method.Create(Context, remover);
 
-            ExtractModifiers(trapFile);
+            PopulateModifiers(trapFile);
             BindComments();
 
             var declSyntaxReferences = IsSourceDeclaration
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/EventAccessor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/EventAccessor.cs
@@ -16,7 +16,7 @@ class EventAccessor : Accessor
         public override void Populate(TextWriter trapFile)
         {
             PopulateMethod(trapFile);
-            ContainingType.ExtractGenerics();
+            ContainingType.PopulateGenerics();
 
             var @event = EventSymbol;
             if (@event == null)
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs
@@ -52,7 +52,7 @@ protected sealed override void Populate(TextWriter trapFile)
             if (Info.ExprValue is string value)
                 trapFile.expr_value(this, value);
 
-            Type.Type.ExtractGenerics();
+            Type.Type.PopulateGenerics();
         }
 
         public override Microsoft.CodeAnalysis.Location ReportingLocation => Location.symbol;
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Field.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Field.cs
@@ -27,15 +27,15 @@ class Field : CachedSymbol<IFieldSymbol>, IExpressionParentEntity
 
         public override void Populate(TextWriter trapFile)
         {
-            ExtractMetadataHandle(trapFile);
-            ExtractAttributes();
-            ContainingType.ExtractGenerics();
-            ExtractNullability(trapFile, symbol.NullableAnnotation);
+            PopulateMetadataHandle(trapFile);
+            PopulateAttributes();
+            ContainingType.PopulateGenerics();
+            PopulateNullability(trapFile, symbol.NullableAnnotation);
 
             Field unboundFieldKey = Field.Create(Context, symbol.OriginalDefinition);
             trapFile.fields(this, (symbol.IsConst ? 2 : 1), symbol.Name, ContainingType, Type.Type.TypeRef, unboundFieldKey);
 
-            ExtractModifiers(trapFile);
+            PopulateModifiers(trapFile);
 
             if (symbol.IsVolatile)
                 Modifier.HasModifier(Context, trapFile, this, "volatile");
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Indexer.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Indexer.cs
@@ -14,7 +14,7 @@ protected Indexer(Context cx, IPropertySymbol init)
 
         public override void Populate(TextWriter trapFile)
         {
-            ExtractNullability(trapFile, symbol.NullableAnnotation);
+            PopulateNullability(trapFile, symbol.NullableAnnotation);
 
             var type = Type.Create(Context, symbol.Type);
             trapFile.indexers(this, symbol.GetName(useMetadataName: true), ContainingType, type.TypeRef, OriginalDefinition);
@@ -50,7 +50,7 @@ public override void Populate(TextWriter trapFile)
                 }
             }
 
-            ExtractModifiers(trapFile);
+            PopulateModifiers(trapFile);
             BindComments();
 
             var declSyntaxReferences = IsSourceDeclaration
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/LocalVariable.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/LocalVariable.cs
@@ -31,7 +31,7 @@ public override void Populate(TextWriter trapFile)
         {
             if (symbol is ILocalSymbol local)
             {
-                ExtractNullability(trapFile, local.NullableAnnotation);
+                PopulateNullability(trapFile, local.NullableAnnotation);
                 if (local.IsRef)
                     trapFile.type_annotation(this, Kinds.TypeAnnotation.Ref);
             }
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs
@@ -12,7 +12,7 @@ public abstract class Method : CachedSymbol<IMethodSymbol>, IExpressionParentEnt
         public Method(Context cx, IMethodSymbol init)
             : base(cx, init) { }
 
-        protected void ExtractParameters(TextWriter trapFile)
+        protected void PopulateParameters(TextWriter trapFile)
         {
             var originalMethod = OriginalDefinition;
             IEnumerable<IParameterSymbol> parameters = symbol.Parameters;
@@ -62,7 +62,7 @@ protected virtual void ExtractInitializers(TextWriter trapFile)
             // so there's nothing to extract.
         }
 
-        void ExtractMethodBody(TextWriter trapFile)
+        void PopulateMethodBody(TextWriter trapFile)
         {
             if (!IsSourceDeclaration)
                 return;
@@ -330,7 +330,7 @@ public static Method Create(Context cx, IMethodSymbol methodDecl)
 
         bool IStatementParentEntity.IsTopLevelParent => true;
 
-        protected void ExtractGenerics(TextWriter trapFile)
+        protected void PopulateGenerics(TextWriter trapFile)
         {
             var isFullyConstructed = IsBoundGeneric;
 
@@ -375,12 +375,12 @@ protected void PopulateMethod(TextWriter trapFile)
         {
             // Common population code for all callables
             BindComments();
-            ExtractAttributes();
-            ExtractParameters(trapFile);
-            ExtractMethodBody(trapFile);
-            ExtractGenerics(trapFile);
-            ExtractMetadataHandle(trapFile);
-            ExtractNullability(trapFile, symbol.ReturnNullableAnnotation);
+            PopulateAttributes();
+            PopulateParameters(trapFile);
+            PopulateMethodBody(trapFile);
+            PopulateGenerics(trapFile);
+            PopulateMetadataHandle(trapFile);
+            PopulateNullability(trapFile, symbol.ReturnNullableAnnotation);
         }
 
         public override TrapStackBehaviour TrapStackBehaviour => TrapStackBehaviour.PushesLabel;
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/OrdinaryMethod.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/OrdinaryMethod.cs
@@ -27,8 +27,8 @@ public IMethodSymbol SourceDeclaration
         public override void Populate(TextWriter trapFile)
         {
             PopulateMethod(trapFile);
-            ExtractModifiers(trapFile);
-            ContainingType.ExtractGenerics();
+            PopulateModifiers(trapFile);
+            ContainingType.PopulateGenerics();
 
             var returnType = Type.Create(Context, symbol.ReturnType);
             trapFile.methods(this, Name, ContainingType, returnType.TypeRef, OriginalDefinition);
@@ -43,7 +43,7 @@ public override void Populate(TextWriter trapFile)
             foreach (var l in Locations)
                 trapFile.method_location(this, l);
 
-            ExtractGenerics(trapFile);
+            PopulateGenerics(trapFile);
             Overrides(trapFile);
             ExtractRefReturn(trapFile);
             ExtractCompilerGenerated(trapFile);
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Parameter.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Parameter.cs
@@ -102,9 +102,9 @@ string Name
 
         public override void Populate(TextWriter trapFile)
         {
-            ExtractAttributes();
-            ExtractNullability(trapFile, symbol.NullableAnnotation);
-            ExtractRefKind(trapFile, symbol.RefKind);
+            PopulateAttributes();
+            PopulateNullability(trapFile, symbol.NullableAnnotation);
+            PopulateRefKind(trapFile, symbol.RefKind);
 
             if (symbol.Name != Original.symbol.Name)
                 Context.ModelError(symbol, "Inconsistent parameter declaration");
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Property.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Property.cs
@@ -24,12 +24,12 @@ public override void WriteId(TextWriter trapFile)
 
         public override void Populate(TextWriter trapFile)
         {
-            ExtractMetadataHandle(trapFile);
-            ExtractAttributes();
-            ExtractModifiers(trapFile);
+            PopulateMetadataHandle(trapFile);
+            PopulateAttributes();
+            PopulateModifiers(trapFile);
             BindComments();
-            ExtractNullability(trapFile, symbol.NullableAnnotation);
-            ExtractRefKind(trapFile, symbol.RefKind);
+            PopulateNullability(trapFile, symbol.NullableAnnotation);
+            PopulateRefKind(trapFile, symbol.RefKind);
 
             var type = Type.Create(Context, symbol.Type);
             trapFile.properties(this, symbol.GetName(), ContainingType, type.TypeRef, Create(Context, symbol.OriginalDefinition));
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statement.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statement.cs
@@ -59,7 +59,7 @@ protected Statement(Context cx, TSyntax stmt, Kinds.StmtKind kind, IStatementPar
             cx.BindComments(this, location.symbol);
         }
 
-        protected override void Populate(TextWriter trapFile)
+        protected sealed override void Populate(TextWriter trapFile)
         {
             trapFile.statements(this, Kind);
             if (Parent.IsTopLevelParent)
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Symbol.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Symbol.cs
@@ -16,26 +16,26 @@ public CachedSymbol(Context cx, T init)
 
         public virtual Type ContainingType => symbol.ContainingType != null ? Type.Create(Context, symbol.ContainingType) : null;
 
-        public void ExtractModifiers(TextWriter trapFile)
+        public void PopulateModifiers(TextWriter trapFile)
         {
             Modifier.ExtractModifiers(Context, trapFile, this, symbol);
         }
 
-        protected void ExtractAttributes()
+        protected void PopulateAttributes()
         {
             // Only extract attributes for source declarations
             if (ReferenceEquals(symbol, symbol.OriginalDefinition))
                 Attribute.ExtractAttributes(Context, symbol, this);
         }
 
-        protected void ExtractNullability(TextWriter trapFile, NullableAnnotation annotation)
+        protected void PopulateNullability(TextWriter trapFile, NullableAnnotation annotation)
         {
             var ta = annotation.GetTypeAnnotation();
             if (ta != Kinds.TypeAnnotation.None)
                 trapFile.type_annotation(this, ta);
         }
 
-        protected void ExtractRefKind(TextWriter trapFile, RefKind kind)
+        protected void PopulateRefKind(TextWriter trapFile, RefKind kind)
         {
             switch (kind)
             {
@@ -125,7 +125,7 @@ public ExpressionSyntax ExpressionBody
 
         public Extraction.Entities.Location Location => Context.Create(ReportingLocation);
 
-        protected void ExtractMetadataHandle(TextWriter trapFile)
+        protected void PopulateMetadataHandle(TextWriter trapFile)
         {
             var handle = MetadataHandle;
 
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/ArrayType.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/ArrayType.cs
@@ -26,8 +26,8 @@ class ArrayType : Type<IArrayTypeSymbol>
         public override void Populate(TextWriter trapFile)
         {
             trapFile.array_element_type(this, Dimension, Rank, element.Type.TypeRef);
-            ExtractType(trapFile);
-            ExtractNullability(trapFile, symbol.ElementNullableAnnotation);
+            PopulateType(trapFile);
+            PopulateNullability(trapFile, symbol.ElementNullableAnnotation);
         }
 
         public override void WriteId(TextWriter trapFile)
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/NamedType.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/NamedType.cs
@@ -65,7 +65,7 @@ public override void Populate(TextWriter trapFile)
                 }
             }
 
-            ExtractType(trapFile);
+            PopulateType(trapFile);
 
             if (symbol.EnumUnderlyingType != null)
             {
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/PointerType.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/PointerType.cs
@@ -24,7 +24,7 @@ public override void WriteId(TextWriter trapFile)
         public override void Populate(TextWriter trapFile)
         {
             trapFile.pointer_referent_type(this, PointedAtType.TypeRef);
-            ExtractType(trapFile);
+            PopulateType(trapFile);
         }
 
         public Type PointedAtType { get; private set; }
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/TupleType.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/TupleType.cs
@@ -38,8 +38,8 @@ public override void WriteId(TextWriter trapFile)
 
         public override void Populate(TextWriter trapFile)
         {
-            ExtractType(trapFile);
-            ExtractGenerics();
+            PopulateType(trapFile);
+            PopulateGenerics();
 
             var underlyingType = NamedType.Create(Context, symbol.TupleUnderlyingType);
             trapFile.tuple_underlying_type(this, underlyingType);
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/Type.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/Type.cs
@@ -78,16 +78,18 @@ static Kinds.TypeKind GetClassType(Context cx, ITypeSymbol t)
             }
         }
 
-        protected void ExtractType(TextWriter trapFile)
+        protected void PopulateType(TextWriter trapFile)
         {
-            ExtractMetadataHandle(trapFile);
-            ExtractAttributes();
+            PopulateMetadataHandle(trapFile);
+            PopulateAttributes();
 
-            using (var tb = new StringWriter())
-            {
-                symbol.BuildDisplayName(Context, tb);
-                trapFile.types(this, GetClassType(Context, symbol), tb.ToString());
-            }
+            trapFile.Write("types(");
+            trapFile.WriteColumn(this);
+            trapFile.Write(',');
+            trapFile.WriteColumn((int)GetClassType(Context, symbol));
+            trapFile.Write(",\"");
+            symbol.BuildDisplayName(Context, trapFile);
+            trapFile.WriteLine("\")");
 
             // Visit base types
             var baseTypes = new List<Type>();
@@ -214,7 +216,7 @@ public void ExtractRecursive()
         /// <summary>
         /// Extracts all members and nested types of this type.
         /// </summary>
-        public void ExtractGenerics()
+        public void PopulateGenerics()
         {
             if (symbol == null || !NeedsPopulation || !Context.ExtractGenerics(this))
                 return;
@@ -245,11 +247,11 @@ public void ExtractGenerics()
             }
 
             if (symbol.BaseType != null)
-                Create(Context, symbol.BaseType).ExtractGenerics();
+                Create(Context, symbol.BaseType).PopulateGenerics();
 
             foreach (var i in symbol.Interfaces)
             {
-                Create(Context, i).ExtractGenerics();
+                Create(Context, i).PopulateGenerics();
             }
         }
 
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/UserOperator.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/UserOperator.cs
@@ -13,7 +13,7 @@ protected UserOperator(Context cx, IMethodSymbol init)
         public override void Populate(TextWriter trapFile)
         {
             PopulateMethod(trapFile);
-            ExtractModifiers(trapFile);
+            PopulateModifiers(trapFile);
 
             var returnType = Type.Create(Context, symbol.ReturnType);
             trapFile.operators(this,
@@ -35,7 +35,7 @@ public override void Populate(TextWriter trapFile)
                     TypeMention.Create(Context, declaration.Type, this, returnType);
             }
 
-            ContainingType.ExtractGenerics();
+            ContainingType.PopulateGenerics();
         }
 
         public override bool NeedsPopulation => Context.Defines(symbol) || IsImplicitOperator(out _);
diff --git a/csharp/ql/src/semmle/code/cil/ConsistencyChecks.qll b/csharp/ql/src/semmle/code/cil/ConsistencyChecks.qll
diff --git a/csharp/ql/src/semmle/code/cil/Method.qll b/csharp/ql/src/semmle/code/cil/Method.qll
diff --git a/csharp/ql/src/semmle/code/cil/Type.qll b/csharp/ql/src/semmle/code/cil/Type.qll

Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,7 @@ class EventAccessor : Accessor`
`16`	`16`	`public override void Populate(TextWriter trapFile)`
`17`	`17`	`{`
`18`	`18`	`PopulateMethod(trapFile);`
`19`		`- ContainingType.ExtractGenerics();`
	`19`	`+ ContainingType.PopulateGenerics();`
`20`	`20`
`21`	`21`	`var @event = EventSymbol;`
`22`	`22`	`if (@event == null)`
Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,7 @@ protected sealed override void Populate(TextWriter trapFile)`
`52`	`52`	`if (Info.ExprValue is string value)`
`53`	`53`	`trapFile.expr_value(this, value);`
`54`	`54`
`55`		`- Type.Type.ExtractGenerics();`
	`55`	`+ Type.Type.PopulateGenerics();`
`56`	`56`	`}`
`57`	`57`
`58`	`58`	`public override Microsoft.CodeAnalysis.Location ReportingLocation => Location.symbol;`
Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,7 @@ protected Indexer(Context cx, IPropertySymbol init)`
`14`	`14`
`15`	`15`	`public override void Populate(TextWriter trapFile)`
`16`	`16`	`{`
`17`		`- ExtractNullability(trapFile, symbol.NullableAnnotation);`
	`17`	`+ PopulateNullability(trapFile, symbol.NullableAnnotation);`
`18`	`18`
`19`	`19`	`var type = Type.Create(Context, symbol.Type);`
`20`	`20`	`trapFile.indexers(this, symbol.GetName(useMetadataName: true), ContainingType, type.TypeRef, OriginalDefinition);`
`@@ -50,7 +50,7 @@ public override void Populate(TextWriter trapFile)`
`50`	`50`	`}`
`51`	`51`	`}`
`52`	`52`
`53`		`- ExtractModifiers(trapFile);`
	`53`	`+ PopulateModifiers(trapFile);`
`54`	`54`	`BindComments();`
`55`	`55`
`56`	`56`	`var declSyntaxReferences = IsSourceDeclaration`