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

Merge latest bits from main branch into extensions branch #72684

Merged
merged 1,084 commits into from
Apr 2, 2024

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Mar 22, 2024

The last two three commits resolve conflicts.

CyrusNajmabadi and others added 30 commits March 21, 2024 12:47
…erMap (dotnet#72596)

Reduce allocations due to resizes in SolutionCompilationState.CreateCompilationTrackerMap. We now use an ImmutableSegmentedDictionary builder to both simplify the code and allocate less. The builder will only create a new dictionary if it's modified, and it does so in a way that doesn't require multiple resizes (which is where the allocation gains from this change are realized).

Additionally, use of the builder allowed a simplification where the modification callback can be changed to a simple action. The call to CreateCompilationTrackerMap now passes in an optimization flag indicating whether the callback needs to occur for empty collections.
Unify C# and VB implementations of TestCompletionInLinkedFiles
Implement ValueBuilder types for immutable collections
[main] Update dependencies from dotnet/arcade
* Create API docs feedback template

This adds the issue template that will be configured for roslyn-api-docs feedback published on docs.microsoft.com. Many of the fields will be applied from the query string of the referrer URL.

* Update .github/ISSUE_TEMPLATE/z-apidocs-feedback.yml

Co-authored-by: Fred Silberberg <fred@silberberg.xyz>

---------

Co-authored-by: Fred Silberberg <fred@silberberg.xyz>
@jjonescz
Copy link
Member

jjonescz commented Mar 27, 2024

Looks like CI run failed: https://dev.azure.com/dnceng-public/public/_build/results?buildId=614874&view=results

I think you might need to merge main again to get #72683 in

@jcouv
Copy link
Member Author

jcouv commented Mar 27, 2024

Thanks for the pointer! I'm going to do a new merge from scratch and will force-push, so that the commits for resolving conflicts remain at the end.

@jcouv jcouv requested a review from jjonescz March 27, 2024 18:06
@@ -131,7 +131,7 @@ public override Conversion GetMethodGroupDelegateConversion(BoundMethodGroup sou
return false;
}

MethodGroupResolution resolution = _binder.ResolveMethodGroup(source, analyzedArguments: null, isMethodGroupConversion: false, ref useSiteInfo);
MethodGroupResolution resolution = _binder.ResolveMethodGroup(source, analyzedArguments: null, ref useSiteInfo, options: OverloadResolution.Options.IsMethodGroupConversion);
Copy link
Member

Choose a reason for hiding this comment

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

Previously isMethodGroupConversion was false and now we are passing option IsMethodGroupConversion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mistake on my part

@@ -21571,7 +21571,7 @@ public static void M(dynamic d)
var model = comp.GetSemanticModel(tree);
var memberAccess = GetSyntax<ElementAccessExpressionSyntax>(tree, "(new C())[d]");
Assert.Equal("System.Int32 C.this[System.Int32 i] { get; }", model.GetSymbolInfo(memberAccess).Symbol.ToTestDisplayString());
Assert.Equal(["System.Int32 C.this[System.Int32 i] { get; }"], model.GetMemberGroup(memberAccess).ToTestDisplayStrings());
Assert.Equal([], model.GetMemberGroup(memberAccess).ToTestDisplayStrings());
Copy link
Member

Choose a reason for hiding this comment

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

Why has this changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the way this test without the extension now behaves in main branch where some changes relative to dynamic were made.
Given that this scenario doesn't actually involve the extension member, I didn't investigate further.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the way this test without the extension now behaves in main branch where some changes relative to dynamic were made. Given that this scenario doesn't actually involve the extension member, I didn't investigate further.

The behavior looks consistent with early bound case (no dynamic involved):

        [Fact]
        public void Test()
        {
            var source = """
C.M(1);
class C
{
    public static void M(int d)
    {
        System.Console.Write((new C())[d]);
    }
    public int this[int i] => 42;
}
""";
            var comp = CreateCompilation(source, targetFramework: TargetFramework.Net70);
            CompileAndVerify(comp, expectedOutput: ("42"), verify: Verification.FailsPEVerify).VerifyDiagnostics();
            var tree = comp.SyntaxTrees.First();
            var model = comp.GetSemanticModel(tree);
            var memberAccess = GetSyntax<ElementAccessExpressionSyntax>(tree, "(new C())[d]");
            Assert.Equal("System.Int32 C.this[System.Int32 i] { get; }", model.GetSymbolInfo(memberAccess).Symbol.ToTestDisplayString());
            Assert.Equal([], model.GetMemberGroup(memberAccess).ToTestDisplayStrings());
        }

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1083)

@jcouv jcouv requested a review from jjonescz April 1, 2024 15:25
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 1084)

@jcouv jcouv merged commit 73e499d into dotnet:features/roles Apr 2, 2024
31 checks passed
@jcouv jcouv deleted the merge-master branch April 2, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.