Extensions: extension marker type name#79308
Merged
jcouv merged 8 commits intodotnet:mainfrom Jul 17, 2025
Merged
Conversation
b02d703 to
cc57b33
Compare
cc57b33 to
8c091ef
Compare
AlekseyTs
reviewed
Jul 10, 2025
| appendType(extensionParameter.Type, builder); | ||
| if (extensionParameter.RefKind != RefKind.None) | ||
| { | ||
| builder.Append('&'); |
Contributor
Contributor
There was a problem hiding this comment.
It looks like this contradicts our decision made on July 2nd:
proposal is to not include refness in extension grouping
AlekseyTs
reviewed
Jul 10, 2025
| builder.Append("("); | ||
| if (this.ExtensionParameter is { } extensionParameter) | ||
| { | ||
| // We intentionally ignore "scoped" and "params" |
Contributor
AlekseyTs
reviewed
Jul 10, 2025
|
|
||
| static void appendTypeWithAnnotation(TypeWithAnnotations type, StringBuilder builder) | ||
| { | ||
| appendType(type.Type, builder); |
Contributor
AlekseyTs
reviewed
Jul 10, 2025
| return; | ||
| } | ||
|
|
||
| if (namedType.Name == "notnull") |
Contributor
AlekseyTs
reviewed
Jul 10, 2025
| } | ||
| } | ||
|
|
||
| static void appendNamedType(TypeSymbol type, StringBuilder builder, NamedTypeSymbol namedType) |
Contributor
AlekseyTs
reviewed
Jul 10, 2025
| { | ||
| builder.Append('('); | ||
| ImmutableArray<string?> elementNames = namedType.TupleElementNames; | ||
| for (int i = 0; i < namedType.TupleElementTypesWithAnnotations.Length; i++) |
Contributor
AlekseyTs
reviewed
Jul 10, 2025
| return; | ||
| } | ||
|
|
||
| if (namedType.IsTopLevelType() && namedType.ContainingNamespace.IsGlobalNamespace) |
Contributor
AlekseyTs
reviewed
Jul 10, 2025
|
|
||
| static void appendTypeConstraints(TypeParameterSymbol typeParam, StringBuilder builder, ref bool needComma) | ||
| { | ||
| var typeConstraintsBuilder = ArrayBuilder<string>.GetInstance(); |
Contributor
AlekseyTs
reviewed
Jul 10, 2025
| static void appendTypeConstraints(TypeParameterSymbol typeParam, StringBuilder builder, ref bool needComma) | ||
| { | ||
| var typeConstraintsBuilder = ArrayBuilder<string>.GetInstance(); | ||
| for (int i = 0; i < typeParam.ConstraintTypesNoUseSiteDiagnostics.Length; i++) |
Contributor
AlekseyTs
reviewed
Jul 10, 2025
|
|
||
| Debug.Assert(callingConventionTypes[i].Name.StartsWith("CallConv", StringComparison.Ordinal)); | ||
|
|
||
| builder.Append(callingConventionTypes[i].Name.Remove(0, "CallConv".Length)); |
Contributor
AlekseyTs
reviewed
Jul 10, 2025
| builder.Append(", "); | ||
| } | ||
|
|
||
| Debug.Assert(callingConventionTypes[i].Name.StartsWith("CallConv", StringComparison.Ordinal)); |
Contributor
Member
Author
There was a problem hiding this comment.
Yes, the logic in UnmanagedCallingConventionTypes only includes modifiers that conform to FunctionPointerTypeSymbol.IsCallingConventionModifier which includes a check for modifierType.Name.StartsWith("CallConv", StringComparison.Ordinal).
AlekseyTs
reviewed
Jul 10, 2025
|
|
||
| builder.Append('('); | ||
| bool needComma = false; | ||
| foreach (TypedConstant argument in attribute.CommonConstructorArguments) |
Contributor
Member
Author
There was a problem hiding this comment.
- named ->
MarkerTypeRawName_22 - default parameter ->
MarkerTypeRawName_28 - paramarray ->
MarkerTypeRawName_45
Contributor
There was a problem hiding this comment.
MarkerTypeRawName_22
It doesn't look like it covers regular named parameter (i.e. not a member initialization). Specifically it would be interesting to cover situation when arguments insource do not match parameter order in constructor's signature.
Contributor
There was a problem hiding this comment.
MarkerTypeRawName_45
It doesn't look like it covers paramarray arguments for the attribute
Member
Author
There was a problem hiding this comment.
MarkerTypeRawName_22
Isn't this what you mean?
extension([My(s: "hello", value: 10, P2 = "hello2", P = 20)] int)
public MyAttribute(int value, string s) { }
Contributor
There was a problem hiding this comment.
Isn't this what you mean?
You are right. I didn't see the second compilation
AlekseyTs
reviewed
Jul 10, 2025
| { | ||
| Debug.Assert(argument.Type is not null); | ||
| builder.Append('('); | ||
| appendType(((PublicModel.TypeSymbol)argument.Type).UnderlyingTypeSymbol, builder); |
Contributor
AlekseyTs
reviewed
Jul 10, 2025
| { | ||
| Debug.Assert(argument.Type is not null); | ||
| builder.Append('('); | ||
| appendType(((PublicModel.TypeSymbol)argument.Type).UnderlyingTypeSymbol, builder); |
Contributor
There was a problem hiding this comment.
If this type is coming from attribute signature, then we cannot make any assumption about the lack of custom modifiers. Also, we probably do not care about any C#-ism in the types from attributes, otherwise the name start depending on an insignificant information that can come from a different assembly and change without user's control. #Closed
Contributor
There was a problem hiding this comment.
Since the type information is not used for all values, consider dropping it completely in all cases.
AlekseyTs
reviewed
Jul 10, 2025
| case TypedConstantKind.Type: | ||
| Debug.Assert(argument.Value is not null); | ||
| builder.Append("typeof("); | ||
| appendType(((PublicModel.TypeSymbol)argument.Value).UnderlyingTypeSymbol, builder); |
Contributor
AlekseyTs
reviewed
Jul 10, 2025
|
|
||
| case TypedConstantKind.Array: | ||
| Debug.Assert(argument.Type is not null); | ||
| if (argument.Values.IsEmpty) |
Contributor
Member
Author
There was a problem hiding this comment.
We needed to make the type explicit when there's no element and I didn't want to repeat the type more when there are elements. That said, I have a flag to skip the cast on primitives now, I'll unify the format
Contributor
There was a problem hiding this comment.
We needed to make the type explicit when there's no element
See my other comment about types coming from attributes
AlekseyTs
reviewed
Jul 10, 2025
| { | ||
| builder.Append("new "); | ||
| appendType(((PublicModel.TypeSymbol)argument.Type).UnderlyingTypeSymbol, builder); | ||
| builder.Append(" { }"); |
Contributor
Member
Author
There was a problem hiding this comment.
The type includes the brackets already
AlekseyTs
reviewed
Jul 10, 2025
| break; | ||
| case float f: | ||
| // Note: we're printing as bits to avoid any loss | ||
| builder.Append(BitConverter.ToInt32(BitConverter.GetBytes(f), startIndex: 0)); |
Contributor
Member
Author
There was a problem hiding this comment.
From discussion with Tanner, ToString loses fidelity on some platforms and that could not be fixed for compat reasons:
"There is no built-in way to produce a roundtrippable floating-point string on .NET Framework/.NET Standard. The implementation is incorrect and cannot be fixed due to back-compat. Simply value.ToString() is roundtrippable on modern .NET -- Some tools attempt to minimize loss on .NET Framework by doing ToString("G17"), but there are some inputs that will still be lossy
Your best bet is to serialize the raw bits instead, which you can get via BitConvert.DoubleToInt64Bits (you'll need to roll your own helper for BitConverter.SingleToInt32Bits on .NET Standard/.NET Framework, the in box API is modern .NET only)"
Member
Author
There was a problem hiding this comment.
Sorry, I'd misunderstood. The suggestion is to add .ToString...
AlekseyTs
reviewed
Jul 10, 2025
| break; | ||
| case double d: | ||
| // Note: we're printing as bits to avoid any loss | ||
| builder.Append(BitConverter.DoubleToInt64Bits(d)); |
Contributor
There was a problem hiding this comment.
AlekseyTs
reviewed
Jul 10, 2025
| break; | ||
| case double d: | ||
| // Note: we're printing as bits to avoid any loss | ||
| builder.Append(BitConverter.DoubleToInt64Bits(d)); |
Contributor
jjonescz
reviewed
Jul 14, 2025
| } | ||
| } | ||
|
|
||
| internal override ScopedKind DeclaredScope => _originalParam.DeclaredScope; |
Member
There was a problem hiding this comment.
sealed override? Applies also to other similar places. #Resolved
| var parameters = functionPointer.Signature.Parameters; | ||
| for (int i = 0; i < parameters.Length; i++) | ||
| appendRefKind(signature.RefKind, builder); | ||
| appendTypeWithAnnotation(signature.ReturnTypeWithAnnotations.WithModifiers([]), builder); |
Member
There was a problem hiding this comment.
Why are we removing modifiers here? #Resolved
Member
Author
There was a problem hiding this comment.
Custom modifiers cannot appear in C# source. But function pointer symbols have them even in source and they are created early. We need to strip them to avoid hitting added assertion that there are no modifiers
| /// <summary> | ||
| /// This name uses a C#-looking format to encode C#-level information of an extension block (ie. arity, constraints, extended type, attributes and C#-isms like tuple names). | ||
| /// It is meant to be hashed to produce the content-based name for the extension marker type. | ||
| /// /// </summary> |
Member
There was a problem hiding this comment.
Suggested change
| /// /// </summary> | |
| /// </summary> | |
| ``` #Resolved |
AlekseyTs
reviewed
Jul 14, 2025
| case TypedConstantKind.Type: | ||
| Debug.Assert(argument.ValueInternal is not null); | ||
| builder.Append("typeof("); | ||
| appendType((TypeSymbol)argument.ValueInternal, builder); |
Contributor
Contributor
For Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol_Extension.cs:268 in bd9632c. [](commit_id = bd9632c, deletion_comment = False) |
AlekseyTs
reviewed
Jul 14, 2025
|
|
||
| static void appendAttributeNamedArgument(KeyValuePair<string, TypedConstant> namedArgument, StringBuilder builder) | ||
| { | ||
| appendIdentifier(namedArgument.Key, builder); |
Contributor
Member
Author
There was a problem hiding this comment.
I don't think so. Could not define properties with same name but different type in IL, so I don't think a signature would add disambiguation
Contributor
Contributor
Perhaps we should match the order from IL, since we are using IL syntax #Closed Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs:9140 in bd9632c. [](commit_id = bd9632c, deletion_comment = False) |
Contributor
|
Done with review pass (commit 4) #Closed |
jjonescz
approved these changes
Jul 15, 2025
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
333fred
added a commit
that referenced
this pull request
Jul 24, 2025
* Support interpolated string handlers in extension blocks (#78425) * Support interpolated string handlers in extension blocks Closes #78137. * A few tests fail PEVerify * Update handling of receivers in static scenarios and add tests for indexers/static methods * Update after merge * Ensure nullable index adjustment happens always, not just when the receiver parameter is used. * Additional il verification progress * PR feedback. The only remaining item is additional testing for ref safety to record the current state. * Additional PR feedback * More testing, suggested simplification * Simplify and more testing. * Feedback * Suggested optimization * Add skipped test for #78433. * Fix loss of nullability attributes when getting the 'constructed reduced from' method. (#79400) * Add tracking issue for disabled assert (#79427) Reference tracking issue #79426 for the disabled assert. * Fix code gen for some compound assignment scenarios involving extension properties. (#79339) * Extensions: extension marker type name (#79308) * Move to xunit.runner.visualstudio 3.1.3 * Avoid eliding pointer-to-ref conversions (#79311) * Avoid eliding pointer to ref conversions * Consider only user-defined locals * Update pre-existing tests * Test evaluation order * Explain user-defined local check * Avoid reusing temps whose refs might be captured (#76009) * Avoid reusing temps whose refs might be captured * Revert some changes * Simplify the heuristic * Avoid reusing any local whose address has been taken * Update tests * Inline a function * Revert unrelated change * Remove non-reusable locals after checking for them * Keep ref count for addressed locals list * Extend an assert * Add high-level comment to MightEscapeTemporaryRefs * Filter non-reusable locals * Simplify `int`s to `bool`s * Replace coalesce with an assert * Mark nested calls as always used * Fix this parameter of nint methods * Test chained call * Simplify by using this parameter symbol * Fix readonly targets * Fixup a test * Remove an unused parameter * Fix nullability after merge * [main] Update dependencies from dotnet/arcade (#79385) [main] Update dependencies from dotnet/arcade - Revert .NET SDK bump - Set NUGET_PACKAGES for bootstrap builds in CI - Set NUGET_PACKAGES for Correctness_Build_Artifacts - Add tracking issue for workarounds - Call nuget restore with the -publish flag - We need to restore where we publish - Add comment for strange restore behavior - Update comment * Fixes rename attribute bug (#79418) * fix bug * update test * simplify * comment * clarity * gracefully handle the document or languageservice being null * Adds EqualityComparer<T>.Create polyfill * Revert compiler changes * Update dependencies from https://github.com/dotnet/arcade build 20250717.3 Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks From Version 10.0.0-beta.25358.3 -> To Version 10.0.0-beta.25367.3 * Move to .NET 10p6 (#79438) * Move to .NET 10p6 Let's try to get ahead of the next one. * Update contributing docs as well. * Allow razor to use SolutionChecksumUpdater (#79373) * Allow razor to use SolutionChecksumUpdater SolutionChecksumUpdater allows incremental updates of sourcetexts in the server process, but was previously limited to just documents in the solution. Instead, by loosening this to allow TextDocuments, razor editing can have incremental updates of the primary razor document when cohosting is turned on. * use is/or instead of ==/|| * Remove dependency on ClassificationOptions and DefinitionItem from Semantic Search service (#79439) * Update insertions for VS release changes * Fix static extension method not showing up on enum type * Add workitem * cleanup * Use ConfigureAwait(true) when in a blocking JTF run call * GetTextSynchronously * Update src/VisualStudio/Core/Def/LanguageService/AbstractLanguageService`2.VsLanguageDebugInfo.cs Co-authored-by: Jason Malinowski <jason@jason-m.com> * Make synchronous * Docs * Allow Razor to create a formatting options, and send it to OOP * Update dependencies from https://github.com/dotnet/arcade build 20250717.4 Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks From Version 10.0.0-beta.25358.3 -> To Version 10.0.0-beta.25367.4 * Remove assumption about a tree shape that compound assignment produces (#79432) * Skip GetProjectXml_FileBasedProgram_SdkTooOld_01 test * Bump Microsoft.VisualStudio.Extensibility.Testing.Xunit to 0.1.796-beta This brings along some fixes needed for it to work under newer versions of xunit.runner.visualstudio. * Reduce path length of Roslyn ServiceHub Services folder (#79460) I've been hitting lots of gold bars in VS indicating various Roslyn features are unavailable when running when F5'ing the roslyn project. I finally spent some time digging into why this happens on my machine, and it turns out that out service hub service paths are just a bit too long. This PR just reduces that path length by 14 characters, which is enough for all the services to be under the 260 character long path limit. * downgrade SDK * Fix Typo * Simplify stale project handling (#79386) * Update System.Memory to prevent bootstrap failure * Include dependencies of System.Memory in VSIX deployment project * remove unnecessary additional reference * Update dependencies from https://github.com/dotnet/arcade build 20250717.5 Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks From Version 10.0.0-beta.25358.3 -> To Version 10.0.0-beta.25367.5 * Use new compiler API * Correctly change return type of partial definition part * Feedback * Move RoslynParallel and ProducerConsumer to threading source package (#79474) * Downgrade SDK * Do not remove usings/imports in code-cleanup when file has syntax errors * Add testws * Find add-using results in SG docs * Docs * Do not place SuppressMessage attributes on an extension block * remove asynchrony from service registration * Ignore all targets of MS.CA.Threading.Package in rebuild (#79495) * Don't mark special compiler synthesized members as obsolete * Add test * Use globs in .projitems for source packages (#79480) * Fix name declaration completion in primary constructor parameters * nrt * nrt * Generate complogs on build failure (#79446) * Generate complogs on build failure This should make it easier to reproduce CI compiler failures locally. * Test bad change for complog creation * Use --yes * Include full exception * Revert "Test bad change for complog creation" This reverts commit a465981. * remove from pkgdefs * Extensions: fix issue with GetParameterOrExtensionParameter (#79402) * Fix exception when requesting additional file diagnostics * Do not do full cleanup when we are formatting a new document The CodeAction.CleanupDocumentAsync was added in #59091 to ensure the document had elastic trivia formatted before we applied the result to the file. It appears the intent was to do just the trivia formatting, but the CleanupDocumentAsync does a lot of expensive operations around simplification and adding imports, which aren't needed in this case. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1540532 * Extensions: add test for function type scenario (#79323) * Reenable analyzer that was disabled when we moved to .Net 10 Preview 5 * Allow captured primary constructor parameters to be still used in base-list * Simplify ancestor checks * Fix 'this' keyword not being recomended where allowed * in progress * Fix 'switch' keyword recomendations after an expression * Fix escaping of enum members in completion * Suggest proper 'Async'-suffixed name for Task-returning symbols * Fix location of 'partial' modifier when generating new partial members * Update VMR orchestrator property (#79517) * Make attachdebugger flag work on non-Windows (#79499) These compiler flags used Debugger.Launch, which only works on windows. I borrowed the equivalent code from the LSP launcher, which already has this functionality (https://github.com/dotnet/roslyn/blob/cf34acbc5d0f64ff87013cdd02ae61cff562daaf/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs#L75-L89). * Downgrade System.Memory to make VS happy * Fix auth issue in PR val builds * Fix * Update Basic.Reference.Assemblies (#79525) * Update Basic.Reference.Assemblies Update to version 1.8.3 of Basic.Reference.Assemblies and make references to .NET 10p6 available in our tests. * Feedback * Feedback * Allow add using to work on Razor generated documents * Improve compiler server/client logging (#79335) * Remove unused constant * Include timestamp in logs * Log messages to msbuild as well * Log server's process ID * Log process creation on Unix * Exclude some messages from the binlog * Fix test loggers * Revert "Exclude some messages from the binlog" This reverts commit 12c1050. * Cleanup * Simplify * Simplify * Simplify * Add tests * Add tests * Simplify * Simplify * initial change * Renames * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Extensions: address or split remaining open issues directly associated with test plan (#79452) * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * Update tests * In progress * in progress * in progress * in progress * in progress * in progress * in progress * in progress * Cleanup split between lexer and sliding-text-window (#79205) Co-authored-by: Jan Jones <jan.jones.cz@gmail.com> * Add docs * Fix name qualification when moving a using alias outside of a file scoped namespace * Fix strucutre out of bounds * Tests * Add Version.Details.props (#79539) * Fix issue with 'use explicit type' and nullable tuples * Add back * Convert ValueTaskFactory methods to static extensions (#79541) * Update logic * Also downgrade System.Numerics.Vectors for VS * Add Enum static extensions (#79546) * Fix main build break * Fix main build break AGAIN * Projects to deploy (#79430) * [main] Source code updates from dotnet/dotnet (#79483) [main] Source code updates from dotnet/dotnet - Workaround cref resolve issue due to new RuntimeHelpers in SCI * [main] Source code updates from dotnet/dotnet (#79563) [main] Source code updates from dotnet/dotnet * Revert "Fix main build break AGAIN (#79559)" This reverts commit e67e2a4, reversing changes made to 1a74e66. * Tweak build break fix * Tweak build break fix * inline test strings * Cleanup test methods * Fix issue where we weren't properly adding elastic trivia to newly generated members * Fix behavior * Add test * Mark SyntaxTokenParser as non-experimental (#79521) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> --------- Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com> Co-authored-by: AlekseyTs <AlekseyTs@users.noreply.github.com> Co-authored-by: Julien Couvreur <julien.couvreur@gmail.com> Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com> Co-authored-by: Jan Jones <janjones@microsoft.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Joey Robichaud <jorobich@microsoft.com> Co-authored-by: Ankita Khera <40616383+akhera99@users.noreply.github.com> Co-authored-by: tmat <tomas.matousek@microsoft.com> Co-authored-by: Tomáš Matoušek <tmat@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Todd Grunke <toddgrun@microsoft.com> Co-authored-by: David Barbet <dabarbet@microsoft.com> Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com> Co-authored-by: Jason Malinowski <jason@jason-m.com> Co-authored-by: David Wengier <david.wengier@microsoft.com> Co-authored-by: Jason Malinowski <jason.malinowski@microsoft.com> Co-authored-by: DoctorKrolic <mapmyp03@gmail.com> Co-authored-by: Matt Mitchell <mmitche@microsoft.com> Co-authored-by: Jan Jones <jan.jones.cz@gmail.com> Co-authored-by: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to test plan #76130