C# 14: Support extension types.#21220
Conversation
csharp/extractor/Semmle.Extraction.CSharp/CodeAnalysisExtensions/SymbolExtensions.cs
Fixed
Show fixed
Hide fixed
6218b5c to
5c44c08
Compare
…meter and make it possible to specify a parameter position offset.
a03f985 to
2a9166f
Compare
…tic generated methods with invocation of extension type member.
extension blocks.extension types.
2a9166f to
02e4a8b
Compare
|
@hvitved : The PR is ready for review. However, I haven't added any upgrade/downgrade scripts yet in case the review leads to a change in the DB scheme. Review on commit by commit basis is encouraged. |
hvitved
left a comment
There was a problem hiding this comment.
Impressive work! Some mostly minor comments
| | @xmllocatable | @asp_element | @namespace | @preprocessor_directive; | ||
|
|
||
| @declaration = @callable | @generic | @assignable | @namespace; | ||
| @declaration = @callable | @generic | @assignable | @namespace | @extension_type; |
There was a problem hiding this comment.
Is this needed? I would think that @extension_type is part of @type, which is part of @generic.
| { | ||
| /// <summary> | ||
| /// Synthetic parameter for extension methods declared using the extension syntax. | ||
| /// That is, we add a synthetic parameter s to IsValid in the following example: |
There was a problem hiding this comment.
backticks around s and IsValid makes this easier to parse.
| switch (ExtensionParameter.RefKind) | ||
| { | ||
| case RefKind.Ref: | ||
| return Parameter.Kind.Ref; | ||
| case RefKind.In: | ||
| return Parameter.Kind.In; | ||
| case RefKind.RefReadOnlyParameter: | ||
| return Parameter.Kind.RefReadOnly; | ||
| default: | ||
| return Parameter.Kind.None; | ||
| } |
There was a problem hiding this comment.
Should we share this logic with the logic in Parameter.cs?
| /// Returns true if this method is a compiler-generated extension method, | ||
| /// and outputs the original extension method declaration. | ||
| /// </summary> | ||
| public static bool TryGetExtensionMethod(this IMethodSymbol method, out IMethodSymbol? declaration) |
There was a problem hiding this comment.
Why not simply public static IMethodSymbol? TryGetExtensionMethod(this IMethodSymbol method)?
| * Either an extension method (`ExtensionMethod`), an extension operator | ||
| * (`ExtensionOperator`) or an extension accessor (`ExtensionAccessor`). | ||
| */ | ||
| abstract class ExtensionCallable extends Callable { |
There was a problem hiding this comment.
We don't want to expose abstract classes, so better to (1) rename it to ExtensionCallableImpl, (2) move it inside internal/Callable.qll, (3) update all existing references to ExtensionCallableImpl, and (4) add here final class ExtensionCallable = ExtensionCallableImpl.
| Call getACall() { this = result.getTarget() } | ||
|
|
||
| /** Holds if this callable is declared in an extension type. */ | ||
| predicate isInExtension() { this.getDeclaringType() instanceof ExtensionType } |
There was a problem hiding this comment.
If you move this inside the Declaration class, it can be used also in the charpreds of ExtensionProperty and ExtensionAccessor.
| * Either a classic extension method (`ClassicExtensionMethod`) or an extension | ||
| * type extension method (`ExtensionTypeExtensionMethod`). | ||
| */ | ||
| abstract class ExtensionMethod extends ExtensionCallable, Method { |
| * ``` | ||
| */ | ||
| class SyntheticExtensionParameterAccess extends ParameterAccess { | ||
| private Parameter p; |
There was a problem hiding this comment.
No need to have this as a field.
| - ["My.Qltest", "K", false, "GetMyFieldOnSyntheticField", "()", "", "Argument[this].SyntheticField[My.Qltest.K.MySyntheticField2].Field[My.Qltest.K.MyField]", "ReturnValue", "value", "manual"] | ||
| - ["My.Qltest", "Library", false, "SetValue", "(System.Object)", "", "Argument[0]", "Argument[this].SyntheticField[X]", "value", "dfc-generated"] | ||
| - ["My.Qltest", "Library", false, "GetValue", "()", "", "Argument[this].SyntheticField[X]", "ReturnValue", "value", "dfc-generated"] | ||
| - ["My.Qltest", "TestExtensions+extension(Object)", false, "Method1", "(System.Object)", "", "Argument[0]", "ReturnValue", "value", "manual"] |
There was a problem hiding this comment.
This ought to use the FQN of the type being extended, i.e. TestExtensions+extension(System.Object)
In this PR we introduce support for
extensiontypes (blocks). The feature is explained in detail here.The feature generalises extensions to operators and properties.
Here is a small example of an declaration and how it can be invoked.
It turns out that Roslyn generates a static method corresponding to the extension method. To avoid extracting multiple methods with identical bodies (which would further complicate the QL implementation) we
IsValid()method, which has the qualified nameMyExtensions.extension(string).IsValid.stoIsValid()corresponding to the receiver parameters. This needs to be synthesised as we can't re-use the parameter from the extension declaration.MyExtensions.IsValidwithMyExtensions.extensions(string).IsValid.DCA looks good.
dotnet/runtimeproject. These extraction errors are because some of the extensions (that we now extract) adds user-defined compound assignment operators (described here), which are not supported yet.