Skip to content
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

C#: Handle some BMN garbage types. #18894

Merged
merged 9 commits into from
Mar 7, 2025
Original file line number Diff line number Diff line change
@@ -31,7 +31,7 @@ public override void Populate(TextWriter trapFile)
{
if (assemblyPath is not null)
{
var isBuildlessOutputAssembly = isOutputAssembly && Context.ExtractionContext.Mode.HasFlag(ExtractorMode.Standalone);
var isBuildlessOutputAssembly = isOutputAssembly && Context.ExtractionContext.IsStandalone;
var identifier = isBuildlessOutputAssembly
? ""
: assembly.ToString() ?? "";
@@ -72,7 +72,7 @@ public static Assembly CreateOutputAssembly(Context cx)

public override void WriteId(EscapingTextWriter trapFile)
{
if (isOutputAssembly && Context.ExtractionContext.Mode.HasFlag(ExtractorMode.Standalone))
if (isOutputAssembly && Context.ExtractionContext.IsStandalone)
{
trapFile.Write("buildlessOutputAssembly");
}
Original file line number Diff line number Diff line change
@@ -133,7 +133,7 @@ public IMethodSymbol? TargetSymbol
.Where(method => method.Parameters.Length >= Syntax.ArgumentList.Arguments.Count)
.Where(method => method.Parameters.Count(p => !p.HasExplicitDefaultValue) <= Syntax.ArgumentList.Arguments.Count);

return Context.ExtractionContext.Mode.HasFlag(ExtractorMode.Standalone) ?
return Context.ExtractionContext.IsStandalone ?
candidates.FirstOrDefault() :
candidates.SingleOrDefault();
}
Original file line number Diff line number Diff line change
@@ -166,7 +166,9 @@ private class UnderlyingTupleTypeFactory : CachedEntityFactory<INamedTypeSymbol,
// Create typerefs for constructed error types in case they are fully defined elsewhere.
// We cannot use `!this.NeedsPopulation` because this would not be stable as it would depend on
// the assembly that was being extracted at the time.
private bool UsesTypeRef => Symbol.TypeKind == TypeKind.Error || SymbolEqualityComparer.Default.Equals(Symbol.OriginalDefinition, Symbol);
private bool UsesTypeRef =>
Symbol.TypeKind == TypeKind.Error ||
SymbolEqualityComparer.Default.Equals(Symbol.OriginalDefinition, Symbol);

public override Type TypeRef => UsesTypeRef ? (Type)NamedTypeRef.Create(Context, Symbol) : this;
}
Original file line number Diff line number Diff line change
@@ -25,6 +25,40 @@ public static bool ConstructedOrParentIsConstructed(INamedTypeSymbol symbol)
symbol.ContainingType is not null && ConstructedOrParentIsConstructed(symbol.ContainingType);
}


/// <summary>
/// A hashset containing the C# contextual keywords that could be confused with types (and typing).
///
/// For the list of all contextual keywords, see
/// https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/#contextual-keywords
/// </summary>
private readonly HashSet<string> ContextualKeywordTypes = [
"dynamic",
"nint",
"nuint",
"var"
];

/// <summary>
/// Returns true in case we suspect this is a broken type.
/// </summary>
/// <param name="symbol">Type symbol</param>
private bool IsBrokenType(ITypeSymbol symbol)
{
if (!Context.ExtractionContext.IsStandalone ||
!symbol.FromSource() ||
symbol.IsAnonymousType)
{
return false;
}

// (1) public class { ... } is a broken type as it doesn't have a name.
// (2) public class var { ... } is an allowed type, but it overrides the `var` keyword for all uses.
// The same goes for other contextual keywords that could be used as type names.
// It is probably a better heuristic to treat these as broken types.
return string.IsNullOrEmpty(symbol.Name) || ContextualKeywordTypes.Contains(symbol.Name);
}

public Kinds.TypeKind GetTypeKind(Context cx, bool constructUnderlyingTupleType)
{
switch (Symbol.SpecialType)
@@ -48,6 +82,9 @@ public Kinds.TypeKind GetTypeKind(Context cx, bool constructUnderlyingTupleType)
if (Symbol.IsBoundNullable())
return Kinds.TypeKind.NULLABLE;

if (IsBrokenType(Symbol))
return Kinds.TypeKind.UNKNOWN;

switch (Symbol.TypeKind)
{
case TypeKind.Class: return Kinds.TypeKind.CLASS;
Original file line number Diff line number Diff line change
@@ -47,7 +47,7 @@ public BinaryLogExtractionContext(string cwd, string[] args, string outputPath,

public static string? GetAdjustedPath(ExtractionContext extractionContext, string sourcePath)
{
if (extractionContext.Mode.HasFlag(ExtractorMode.BinaryLog)
if (extractionContext.IsBinaryLog
&& extractionContext is BinaryLogExtractionContext binaryLogExtractionContext
&& binaryLogExtractionContext.GetAdjustedPath(sourcePath) is string adjustedPath)
{
Original file line number Diff line number Diff line change
@@ -267,7 +267,7 @@ private void Populate(ISymbol? optionalSymbol, Entities.CachedEntity entity)

bool duplicationGuard, deferred;

if (ExtractionContext.Mode is ExtractorMode.Standalone)
if (ExtractionContext.IsStandalone)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, that this is not semantically equivalent! But the original code looks like a mistake.

{
duplicationGuard = false;
deferred = false;
@@ -376,7 +376,7 @@ private void ExtractionError(InternalError error)

private void ReportError(InternalError error)
{
if (!ExtractionContext.Mode.HasFlag(ExtractorMode.Standalone))
if (!ExtractionContext.IsStandalone)
throw error;

ExtractionError(error);
Original file line number Diff line number Diff line change
@@ -15,6 +15,8 @@ public class ExtractionContext
public ExtractorMode Mode { get; }
public string OutputPath { get; }
public IEnumerable<CompilationInfo> CompilationInfos { get; }
public bool IsStandalone => Mode.HasFlag(ExtractorMode.Standalone);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need a public Mode property? Can it be made private?

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, it the value itself is written to trap outside this file (it is written to file_extraction_mode).

public bool IsBinaryLog => Mode.HasFlag(ExtractorMode.BinaryLog);

/// <summary>
/// Creates a new extractor instance for one compilation unit.
2 changes: 2 additions & 0 deletions csharp/ql/lib/semmle/code/csharp/Type.qll
Original file line number Diff line number Diff line change
@@ -1214,6 +1214,8 @@ class ArglistType extends Type, @arglist_type {
class UnknownType extends Type, @unknown_type {
/** Holds if this is the canonical unknown type, and not a type that failed to extract properly. */
predicate isCanonical() { types(this, _, "<unknown type>") }

override string getAPrimaryQlClass() { result = "UnknownType" }
}

/**
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Broken type without a name.
public class { }

// Legal declaration, but we want don't want to use it.
public class var { }

public class C
{
public string Prop { get; set; }
}


public class Program
{
public static void Main()
{
C x1 = new C();
string y1 = x1.Prop;

var x2 = new C(); // Has type `var` as this overrides the implicitly typed keyword `var`.
var y2 = x2.Prop; // Unknown type as `x2` has type `var`.

C2 x3 = new C2(); // Unknown type.
var y3 = x3.Prop; // Unknown property of unknown type.

string s = x1.Prop + x3.Prop;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
| BrokenTypes.cs:2:14:2:13 | call to constructor Object | object | ObjectType |
| BrokenTypes.cs:5:14:5:16 | call to constructor Object | object | ObjectType |
| BrokenTypes.cs:7:14:7:14 | call to constructor Object | object | ObjectType |
| BrokenTypes.cs:13:14:13:20 | call to constructor Object | object | ObjectType |
| BrokenTypes.cs:17:11:17:12 | access to local variable x1 | C | Class |
| BrokenTypes.cs:17:11:17:22 | C x1 = ... | C | Class |
| BrokenTypes.cs:17:16:17:22 | object creation of type C | C | Class |
| BrokenTypes.cs:18:16:18:17 | access to local variable y1 | string | StringType |
| BrokenTypes.cs:18:16:18:27 | String y1 = ... | string | StringType |
| BrokenTypes.cs:18:21:18:22 | access to local variable x1 | C | Class |
| BrokenTypes.cs:18:21:18:27 | access to property Prop | string | StringType |
| BrokenTypes.cs:20:13:20:14 | access to local variable x2 | var | UnknownType |
| BrokenTypes.cs:20:13:20:24 | var x2 = ... | var | UnknownType |
| BrokenTypes.cs:20:18:20:24 | (...) ... | var | UnknownType |
| BrokenTypes.cs:20:18:20:24 | object creation of type C | C | Class |
| BrokenTypes.cs:21:13:21:14 | access to local variable y2 | var | UnknownType |
| BrokenTypes.cs:21:13:21:24 | var y2 = ... | var | UnknownType |
| BrokenTypes.cs:21:18:21:19 | access to local variable x2 | var | UnknownType |
| BrokenTypes.cs:21:18:21:24 | (...) ... | var | UnknownType |
| BrokenTypes.cs:21:18:21:24 | access to property (unknown) | | UnknownType |
| BrokenTypes.cs:23:12:23:13 | access to local variable x3 | <unknown type> | UnknownType |
| BrokenTypes.cs:23:12:23:24 | <unknown type> x3 = ... | <unknown type> | UnknownType |
| BrokenTypes.cs:23:17:23:24 | object creation of type <unknown type> | <unknown type> | UnknownType |
| BrokenTypes.cs:24:13:24:14 | access to local variable y3 | var | UnknownType |
| BrokenTypes.cs:24:13:24:24 | var y3 = ... | var | UnknownType |
| BrokenTypes.cs:24:18:24:19 | access to local variable x3 | <unknown type> | UnknownType |
| BrokenTypes.cs:24:18:24:24 | (...) ... | var | UnknownType |
| BrokenTypes.cs:24:18:24:24 | access to property (unknown) | | UnknownType |
| BrokenTypes.cs:26:16:26:16 | access to local variable s | string | StringType |
| BrokenTypes.cs:26:16:26:36 | String s = ... | string | StringType |
| BrokenTypes.cs:26:20:26:21 | access to local variable x1 | C | Class |
| BrokenTypes.cs:26:20:26:26 | access to property Prop | string | StringType |
| BrokenTypes.cs:26:20:26:36 | (...) ... | string | StringType |
| BrokenTypes.cs:26:20:26:36 | ... + ... | | UnknownType |
| BrokenTypes.cs:26:30:26:31 | access to local variable x3 | <unknown type> | UnknownType |
| BrokenTypes.cs:26:30:26:36 | access to property (unknown) | | UnknownType |
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import csharp

from Expr e, Type t
where e.fromSource() and t = e.getType()
select e, t.toStringWithTypes(), t.getAPrimaryQlClass()
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
semmle-extractor-options: --standalone