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

Completion sugests items that do not satisfy generic constraint #39689

Open
pakrym opened this issue Nov 5, 2019 · 11 comments
Open

Completion sugests items that do not satisfy generic constraint #39689

pakrym opened this issue Nov 5, 2019 · 11 comments
Labels
Area-Compilers Bug IDE-IntelliSense Regression
Milestone

Comments

@pakrym
Copy link
Contributor

@pakrym pakrym commented Nov 5, 2019

Version Used: Version 16.5.0 Preview 1.0 [29501.166.master]

Steps to Reproduce:

  1. Create Console app
  2. Add Azure.Storage.Blobs Nuget package
  3. Open intellisense on args.

Expected Behavior:

Extension methods that don't satisfy the generic constraint are not shown.

Actual Behavior:

AddBlobServiceClient is shown

image

        /// <summary>
        /// Registers a <see cref="BlobServiceClient"/> instance with connection options loaded from the provided <paramref name="configuration"/> instance.
        /// </summary>
        public static IAzureClientBuilder<BlobServiceClient, BlobClientOptions> AddBlobServiceClient<TBuilder, TConfiguration>(this TBuilder builder, TConfiguration configuration)
            where TBuilder : IAzureClientFactoryBuilderWithConfiguration<TConfiguration>
        {
            return builder.RegisterClientFactory<BlobServiceClient, BlobClientOptions>(configuration);
        }
@Dreamescaper
Copy link
Contributor

@Dreamescaper Dreamescaper commented Nov 5, 2019

That does not look like native VS IntelliSense. Is that Resharper?

@pakrym
Copy link
Contributor Author

@pakrym pakrym commented Nov 5, 2019

My bad, reproes with R# disabled:

image

@sharwell
Copy link
Member

@sharwell sharwell commented Nov 5, 2019

@ivanbasov This sounds like #37780 but was reported on 16.5 internal preview.

@ivanbasov
Copy link
Contributor

@ivanbasov ivanbasov commented Nov 5, 2019

@gafter this maybe related with #38109 . Maybe more work should be done on the compiler side. Please take a look.

@KirillOsenkov
Copy link
Member

@KirillOsenkov KirillOsenkov commented Apr 23, 2020

@jinujoseph this has been a reoccurring issue and would be nice to fix.

We first saw this in 16.3, then it seemed like it was fixed, now it's reoccurring again in 16.5.

@KirillOsenkov
Copy link
Member

@KirillOsenkov KirillOsenkov commented Apr 23, 2020

image

@jinujoseph jinujoseph assigned genlu and unassigned jinujoseph Apr 23, 2020
@jinujoseph jinujoseph added this to the 16.7.P1 milestone Apr 23, 2020
@jinujoseph jinujoseph removed this from the 16.7.P1 milestone Jun 1, 2020
@jinujoseph jinujoseph added this to the 16.7 milestone Jun 1, 2020
@genlu genlu removed this from the 16.7 milestone Jul 30, 2020
@genlu genlu added this to the 16.8.P1 milestone Jul 30, 2020
@jinujoseph jinujoseph removed this from the 16.8.P1 milestone Sep 3, 2020
@jinujoseph jinujoseph added this to the 16.8 milestone Sep 3, 2020
@pakrym
Copy link
Contributor Author

@pakrym pakrym commented Oct 1, 2020

This bug causes some extension methods from Azure SDKs to be displayed in IntelliSense for every type. We have Azure customers being confused and filing issued about it: Azure/azure-sdk-for-net#15608

Is there any way we can prioritize this issue as it has a very real-world impact on customers?

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Oct 1, 2020

@genlu ptal.

@genlu
Copy link
Member

@genlu genlu commented Oct 1, 2020

This seems to be a compiler issue. ReduceExtensionMethod is unable to filter out the the symbol that doesn't fulfill the constraint like this

    public interface ISomeInterface<T>
    { }

    public static class Ext
    {       
        public static bool SomeExtMethod<T1, T2>(this T1 builder, T2 x)
            where T1 : ISomeInterface<T2>
        {
            return true;
        }
    }
    public class Bar
    {
        void M(string[] s)
        {
            s.$$
        }
    }

FYI @dotnet/roslyn-compiler

@genlu genlu removed the Area-IDE label Oct 1, 2020
@AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Oct 2, 2020

This is by design. We cannot check constraints that depend on type parameters that cannot be inferred from this argument.

@AlekseyTs AlekseyTs added the Resolution-By Design label Oct 2, 2020
@jaredpar
Copy link
Member

@jaredpar jaredpar commented Oct 2, 2020

Adding in a bit of history to issue. The idea of how to handle extension methods and dependent type parameters first came up in #11166. This was based on user feedback and the decision was to ignore dependent constraints when considering extension methods. The motivating scenario is very similar to the one laid out here but with class constraints instead of interface ones:

public static class Extensions {
  public static BC SetMember<BC, TMember>(this BC This, TMember NewValue) 
     where BC : BaseClass<TMember> { }
}

The behavior of including these methods got reverted during the C# 7.3 development as a part of implementing the unmanaged constraint. That resulted in issue #37780 asking us to restore this behavior which was done in PR #37022.

Note: the scenarios are all a bit different in the specific details but they are all essentially oscillating on the idea of how we should handle constraints in extension method reduction. Particularly when they are not fully inferable when dotting off the receiver.

Overall though this is a case where the compiler is being asked to operate with imperfect information. Hence we end up with a heuristic vs. more definitive behavior. There were some discussions about how we could tune this to make this case better without breaking the existing cases by special casing interfaces to a degree. This is not a trivial fix though and it's unclear how grokable this is going to be for end users where as the behavior today is fairly easy to explain. Reactivating the issue for the purpose of exploring that idea though and seeing where we end up.

Note: readers should note that overall this API pattern is decidedly unfriendly for VB users as they must resort to explicit type parameters here.

@jaredpar jaredpar reopened this Oct 2, 2020
@jaredpar jaredpar removed the Resolution-By Design label Oct 2, 2020
@jaredpar jaredpar removed this from the 16.8 milestone Oct 2, 2020
@jaredpar jaredpar added this to the 16.9 milestone Oct 2, 2020
@jinujoseph jinujoseph removed this from the 16.9 milestone Jun 1, 2021
@jinujoseph jinujoseph added this to the 17.0 milestone Jun 1, 2021
@jaredpar jaredpar removed this from the 17.0 milestone Aug 3, 2021
@jaredpar jaredpar added this to the Backlog milestone Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug IDE-IntelliSense Regression
Projects
None yet
Development

No branches or pull requests

10 participants