-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Extensions: address or split remaining open issues directly associated with test plan #79452
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
Conversation
b7c6cd8
to
7312e4f
Compare
e86b2f1
to
e4260cc
Compare
e4260cc
to
e62a6e7
Compare
📝 See MakeDeconstructInvocationExpression line 661 #Closed Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:24200 in 8d46bd8. [](commit_id = 8d46bd8, deletion_comment = True) |
@@ -1518,7 +1518,7 @@ private TypeWithAnnotations MethodGroupReturnType( | |||
returnType: null, | |||
callingConventionInfo: in callingConventionInfo); | |||
|
|||
Debug.Assert(!resolution.IsNonMethodExtensionMember(out _)); | |||
Debug.Assert(!resolution.IsNonMethodExtensionMember(out _)); // we wouldn't be looking at a method group in the first place if the source resolved to a non-method extension member |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to best explain this. When we bind a member access, if the method group could resolve to a non-method extension member access, then we produce that instead of a method group. So we wouldn't get here with a method group if we were dealing with a method group that resolved to a non-method extension member.
@@ -10649,7 +10650,7 @@ internal MethodGroupResolution ResolveMethodGroup( | |||
return ResolveMethodGroup( | |||
node, node.Syntax, node.Name, analyzedArguments, ref useSiteInfo, | |||
options, | |||
acceptOnlyMethods: true, // Tracked by https://github.com/dotnet/roslyn/issues/76130 : Confirm this value is appropriate for all consumers of the enclosing method and test effect of this value for all of them | |||
acceptOnlyMethods: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not observable. All the callers of this method have a Debug.Assert(!resolution.IsNonMethodExtensionMember(out _));
with a comment (being discussed elsewhere).
I think full resolution (ie. acceptOnlyMethods: false
) should be the default mode and the flag should only be set for special cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say, the old behavior makes sense to me. We pass true
here and the consumers assert that result corresponds to the value. I do not like the new code, because it "hopes" it would get expected result despite this value. I would be fine with making this a pass-through parameter (rather than a constant) and explicitly passing true
from the existing call-sites. Then we wouldn't nee all those comments that confuse me a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion it is better to ask for a specific result, perhaps, explaining why we ask that, instead of hoping we get what we want, explaining why we think we would get that.
@@ -10989,6 +10969,17 @@ static bool isCandidateUnique(ref MethodSymbol? method, MethodSymbol candidate) | |||
} | |||
} | |||
|
|||
private static bool IsStaticInstanceMismatch(BoundExpression receiver, Symbol extensionMember) | |||
{ | |||
bool memberCountsAsStatic = extensionMember is MethodSymbol { IsExtensionMethod: true } ? false : extensionMember.IsStatic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool memberCountsAsStatic = extensionMember is MethodSymbol { IsExtensionMethod: true } ? false : extensionMember.IsStatic;
This logic is too special for a method with such "innocent" name. It looks like the method is meant to be used only under specific conditions. Consider renaming to avoid an accidental misuse of the method. #Closed
continue; | ||
} | ||
|
||
// Note: we only care about methods since we're already decided this is a method group (ie. not resolving to some other kind of extension member) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- that's the behavior we have chosen for determining unique signature in newer LangVer (see
GetUniqueSignatureFromMethodGroup
line 11090 below) - that's consistent with how lookup works in general (we work up inheritance chain and if we get a method first, we'll keep looking at other methods to complete the method group, ignoring non-methods)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I misunderstood the question. If we get here, the caller is asking what is the function type of this method group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it would be clearer to say that we decided that we want to extract signature from candidate methods. The term "method group" is somewhat confusing with respect to extensions, since it can resolve to a non-method.
@@ -35329,6 +35407,8 @@ static class E | |||
var symbol = model.GetDeclaredSymbol(extensionParameter); | |||
Assert.Equal(SymbolKind.Parameter, symbol.Kind); | |||
Assert.Equal("System.Int32 i", symbol.ToTestDisplayString()); | |||
|
|||
Assert.Equal("E", model.GetEnclosingSymbol(extensionParameter.SpanStart).ToTestDisplayString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason behind removal of this comment is not obvious #Closed Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:24200 in 8d46bd8. [](commit_id = 8d46bd8, deletion_comment = True) |
@@ -718,6 +718,7 @@ private NamedTypeSymbol MakeAcyclicBaseType(BindingDiagnosticBag diagnostics) | |||
} | |||
else if (typeKind == TypeKind.Extension) | |||
{ | |||
_ = compilation.GetSpecialType(SpecialType.System_Object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's meant to produce a diagnostic, but doesn't... Fixed,, added test and stepped through. Thanks
@@ -288,9 +288,9 @@ Cci.ITypeReference Cci.ITypeDefinition.GetBaseClass(EmitContext context) | |||
Debug.Assert(((Cci.ITypeReference)this).AsTypeDefinition(context) != null); | |||
NamedTypeSymbol baseType = AdaptedNamedTypeSymbol.BaseTypeNoUseSiteDiagnostics; | |||
|
|||
if (AdaptedNamedTypeSymbol.IsScriptClass || AdaptedNamedTypeSymbol.IsExtension) // Tracked by https://github.com/dotnet/roslyn/issues/76130 : we should have checked the presence of System.Object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Tracked by #76130 : we should have checked the presence of System.Object
I would expect to see a test added. #Closed
@@ -34604,6 +34770,86 @@ public void M() { } | |||
CompileAndVerify(comp, expectedOutput: ExpectedOutput("'int' does not contain a definition for 'M'"), verify: Verification.FailsPEVerify); | |||
} | |||
|
|||
[Fact] | |||
public void Dynamic_07() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand the comment. In Dynamic_07
the parameter of the local function is dynamic
and the invocation uses runtime binder. Each of these tests goes through LocalRewriter.VisitDynamicInvocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand the comment.
What dynamic invocation you intend to test?
Done with review pass (commit 23) |
Just to confirm, did you see the comment I'd left? 📝 See MakeDeconstructInvocationExpression line 661 In reply to: 3096811863 Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:24200 in 8d46bd8. [](commit_id = 8d46bd8, deletion_comment = True) |
What am I supposed to see there? In reply to: 3097572916 Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:24200 in 8d46bd8. [](commit_id = 8d46bd8, deletion_comment = True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 26)
comp = CreateCompilation(src, references: [libRef], parseOptions: TestOptions.Regular13); | ||
CompileAndVerify(comp, expectedOutput: "ran").VerifyDiagnostics(unnecessaryDirective); | ||
|
||
if (!CompilationExtensions.EnableVerifyUsedAssemblies) // Tracked by https://github.com/dotnet/roslyn/issues/78968 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlekseyTs FYI, the used assemblies leg hit an issue in VerifyUsedAssemblyReferences
(line 1843, reporting an unexpected diagnostic that namespace N cannot be found when the set of referenced assemblies is shrunk).
The strange thing is that VerifyDiagnostics
doesn't complain about unnecessary using but GetUsedAssemblyReferences()
doesn't report the assembly containing namespace N
as used.
I will investigate in a follow-up.
* 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 <[email protected]> * 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 <[email protected]> * 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] <[email protected]> --------- Co-authored-by: Cyrus Najmabadi <[email protected]> Co-authored-by: AlekseyTs <[email protected]> Co-authored-by: Julien Couvreur <[email protected]> Co-authored-by: Joey Robichaud <[email protected]> Co-authored-by: Jan Jones <[email protected]> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Joey Robichaud <[email protected]> Co-authored-by: Ankita Khera <[email protected]> Co-authored-by: tmat <[email protected]> Co-authored-by: Tomáš Matoušek <[email protected]> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Todd Grunke <[email protected]> Co-authored-by: David Barbet <[email protected]> Co-authored-by: Cyrus Najmabadi <[email protected]> Co-authored-by: Jason Malinowski <[email protected]> Co-authored-by: David Wengier <[email protected]> Co-authored-by: Jason Malinowski <[email protected]> Co-authored-by: DoctorKrolic <[email protected]> Co-authored-by: Matt Mitchell <[email protected]> Co-authored-by: Jan Jones <[email protected]> Co-authored-by: Djuradj Kurepa <[email protected]> Co-authored-by: Copilot <[email protected]>
Removes remaining references to test plan #76130
Closes #77933
This is best reviewed commit by commit.
Corresponding spec update: dotnet/csharplang#9535