dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.1k stars 4.04k forks source link

Collection expressions are suggested for types where it doesn't make sense, and their error messages show inconsistent types #73655

Open cremor opened 6 months ago

cremor commented 6 months ago

Version Used:

Steps to Reproduce: See comments in the sample:

using System;
using System.Collections;
using System.Collections.Generic;

namespace ConsoleApp1;

internal class Program
{
    private readonly InlineValidator<DateTime> _validator;

    public Program()
    {
        // This shows "IDE0028 Collection initialization can be simplified", but only if the Add method below exists.
        // Should it really show this message? I think using a collection expression here is very weird.
        _validator = new InlineValidator<DateTime>();

        // Applying the code fix results in the following working, but weird, code.
        // This still compiles if the Add method below does not exist.
        _validator = [];

        // This calls the Add method below, but should it?
        // The type of the parameter is something completely different than the type of the IEnumerable element.
        // If the Add method below does not exist, then this shows error "CS1061 'InlineValidator<DateTime>' does not
        // contain a definition for 'Add' and no accessible extension method 'Add' accepting a first argument of type
        // 'InlineValidator<DateTime>' could be found".
        // This makes even less sense. Why does it try to find an Add method with parameter type
        // InlineValidator<DateTime>? Shouldn't it only be parameter type DateTime or IValidationRule (depending on
        // whether it should be the generic type of the class itself, or the IEnumerable type)?
        _validator = [null!];

        // This shows error "CS0029 Cannot implicitly convert type 'int' to 'ConsoleApp1.IValidationRule'.
        // This is different from the line above (when the Add method exists). There the compiler passed null as the
        // Add method parameter type (Func). But now it tries to convert it to the IEnumerable element type? Why?
        //_validator = [1];
    }
}

public class InlineValidator<T> : IEnumerable<IValidationRule>
{
    public void Add(Func<int, bool> someFunc)
    {
        someFunc(1);
    }

    public IEnumerator<IValidationRule> GetEnumerator()
    {
        throw new NotImplementedException();
    }
    IEnumerator IEnumerable.GetEnumerator()
    {
        throw new NotImplementedException();
    }
}

public interface IValidationRule;

Sample in SharpLab

My actual project uses https://github.com/FluentValidation/FluentValidation. The sample contains simplified versions of the FluentValidation classes.

Diagnostic Id: Initially "IDE0028: Collection initialization can be simplified". But the following error messages are shown if you modify the sample as explained in the comments:

Expected Behavior: I don't expect to see "IDE0028: Collection initialization can be simplified" for new InlineValidator<DateTime>(). I think this should only be shown if the parameter type of the Add method matches the collection type. I expect consistant parameter/element types shown in the error messages if you modify the sample as explained in the comments.

Actual Behavior: See comments in the sample code.

dotnet-issue-labeler[bot] commented 6 months ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

RikkiGibson commented 6 months ago

Thanks for filing this issue.

cremor commented 6 months ago

Yes, I can still reproduce this with the latest preview versions of VS and the .NET SDK. I've updated the issue description to include this information. And I've also directly included the sample code as requested.

RikkiGibson commented 6 months ago

I'm not sure right now whether to classify as a compiler or IDE (suggest using collection-exprs) issue. I'll have to spend a little more time paging in what's going on to do so.

CyrusNajmabadi commented 6 months ago

@RikkiGibson this is likely on hte IDE side. However, the core issue is that there are no APis the compiler exposes to know anything here. So we basically have had to reimplement all the logic aronud the rules in the language to know if somethign is a legal collectoin-expr type. that wasn't tenable originally, and it gets even harder as we add more complex and involved rules for collection types. As mentioned originally, the IDE needs apis to know if something is a legal collection expr type. Since that logic already exists in the compiler, we wouldn't have to duplicate it and we could defer directly to that to know if we can even bother offering collection exprs at all.

cremor commented 6 months ago

this is likely on hte IDE side

Aren't the error messages coming from the compiler?