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

Have compiler return semantic errors in lambdas even in the presence of syntax errors #1867

Closed
jmarolf opened this issue Apr 8, 2015 · 8 comments
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. Feature Request Language-C#
Milestone

Comments

@jmarolf
Copy link
Contributor

jmarolf commented Apr 8, 2015

Today, the compiler does not return semantic errors inside lambdas in the IDE if there are syntax errors. This causes several quick fix scenarios to fail inside lambdas (for example #1239, #1241, and #1744 are all caused by this problem).

Consider the code sample below from issue #1239:

using System.Threading.Tasks;

class C {
    C() {
        Task.Run(() => {
            new C(0)
        });
    }
}

new C(0) will never be squiggled by the IDE because of the lack of a semicolon. This makes quick-fixes that could generate a constructor unable to run because there is no error to trigger them.

By having the compiler return semantic errors in lambdas even in the presence of syntax errors, many more IDE scenarios will "Just Work"

@gafter gafter added this to the 1.1 milestone Apr 16, 2015
jmarolf added a commit to jmarolf/roslyn that referenced this issue Apr 27, 2015
The compiler does not generate semantic errors inside lambdas in the
presence of syntactic errors causing several features to not work in
lambdas with syntax errors.  The bug for this
([1867](dotnet#1867)) was moved to
milestone 1.1 so we are going to use an analyzer in the interim for 1.0.

1. We now check for IncompleteMemberSyntax nodes and
LambdaExpressionSyntax nodes which contain syntax diagnostics on any of
their descendant nodes.
2. We report both unbound identifier names and constructors that the
compiler reports as binding, but which fail overload resolution
(actually don't exist).

Performance considerations should be mitigated by only doing these
checks only lambdas with syntax errors.

Other notes:

- Renamed analyzer to UnboundIdentifier instead of AddImport since it is
being used in more places than just the AddImport fixer
- Updated the DiagnosticDescriptor for this analyzer to take
localizeable strings.

Fixes dotnet#1744
Fixes dotnet#1241
Fixes dotnet#1239
@gafter gafter modified the milestones: 1.2, 1.1 Aug 15, 2015
@jaredpar jaredpar added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Nov 30, 2015
@jaredpar jaredpar modified the milestones: 2.0, 1.2 Nov 30, 2015
@CyrusNajmabadi
Copy link
Member

Note: whenever this is fixed we need to remove the UnboundIdentifiersDiagnosticAnalyzerBase type and ensure all AddUsing tests (and other tests that depend on this type) still continue passing. Right now, this type is a hack at the IDE layer to get and display reasonable semantic errors to the user and to drive our normal features. This should not be something the IDE is responsible for doing. Users should be getting these errors, even in the presence of a syntax error in a lambda, and our features should not have to list for syntax errors to drive semantic features.

@gafter gafter modified the milestones: 1.2, 2.0 Dec 4, 2015
gafter added a commit to gafter/roslyn that referenced this issue Dec 4, 2015
@gafter gafter added 4 - In Review A fix for the issue is submitted for review. and removed 3 - Working labels Dec 4, 2015
@gafter gafter added Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented and removed 4 - In Review A fix for the issue is submitted for review. labels Dec 11, 2015
@CyrusNajmabadi
Copy link
Member

Reactivating. Here are cases where we still don't get errors from the compiler:

using System.Linq;
class C { C() { "".Select(() => { new Byte  //<-- no error on Byte
class D { D() { "".Select(() => { new Byte  ( ) }   //<-- no error on Byte

class Program
{
    static void Main(string[] args)
    {
        args[0].Any(x => IBindCtx  // <-- no error on IBindCtx
        string a;
    }
}

@gafter gafter added 2 - Ready and removed Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented labels Dec 16, 2015
@gafter
Copy link
Member

gafter commented Dec 17, 2015

For the first two cases, the compiler certainly does produce a semantic error about Byte not found. But I'll be sure to add a test case for them.

Working on the third case.

@gafter
Copy link
Member

gafter commented Dec 17, 2015

OK, in the third case we have a lambda of the form

e.Any(x => identifier string identifier; // and more broken syntax

and you're complaining that we don't give an error that we can't bind the first identifier? What are you imagining the compiler would be trying to bind it for? It simply doesn't line up with any recognizable expression form.

@gafter
Copy link
Member

gafter commented Dec 17, 2015

Here is a better repro for the third case:

        args[0].Any(x => IBindCtx, foo foo);

Here we should give an error that we can't bind IBindCtx.

gafter added a commit to gafter/roslyn that referenced this issue Dec 17, 2015
…ils.

Also reduce cascaded diagnostics in lambdas inside queries.
Fixes dotnet#1867
@gafter gafter added Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. 4 - In Review A fix for the issue is submitted for review. and removed 2 - Ready labels Dec 17, 2015
@gafter
Copy link
Member

gafter commented Dec 17, 2015

OK, PR #7555 addresses all the new cases.

gafter added a commit to gafter/roslyn that referenced this issue Dec 17, 2015
…ils.

Also reduce cascaded diagnostics in lambdas inside queries.
Fixes dotnet#1867
@gafter
Copy link
Member

gafter commented Dec 30, 2015

@CyrusNajmabadi OK, all fixed. I'm eager to catch as many of these cases as possible. You're welcome to open new issues for any new cases you discover.

@CyrusNajmabadi
Copy link
Member

@gafter Will do. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. Feature Request Language-C#
Projects
None yet
Development

No branches or pull requests

5 participants