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.

Sign up for GitHub

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

Improved error recovery for lambdas in a call with extra arguments. #11807

Merged
merged 3 commits into from
Jun 13, 2016
Merged

Improved error recovery for lambdas in a call with extra arguments. #11807

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
567d12f
Improved error recovery for lambdas in a call with extra arguments.
gafter Jun 7, 2016
947f968
Merge branch 'master' of https://github.com/dotnet/roslyn into master…
gafter Jun 8, 2016
dfa0aeb
Tests for #5498 and #11358 that do not have syntax errors.
gafter Jun 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ private enum Dependency
private Dependency[,] _dependencies; // Initialized lazily
private bool _dependenciesDirty;

/// <summary>
/// For error recovery, we allow a mismatch between the number of arguments and parameters
/// during type inference. This sometimes enables inferring the type for a lambda parameter.
/// </summary>
private int Length => System.Math.Min(_arguments.Length, _formalParameterTypes.Length);
Copy link
Member

Choose a reason for hiding this comment

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

Can we name this something more descriptive?

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

AmountOfArgumentsToConsider?

In reply to: 66173546 [](ancestors = 66173546)


public static MethodTypeInferenceResult Infer(
Binder binder,
ImmutableArray<TypeParameterSymbol> methodTypeParameters,
Expand Down Expand Up @@ -524,14 +530,13 @@ private void InferTypeArgsFirstPhase(Binder binder, ref HashSet<DiagnosticInfo>
{
Debug.Assert(!_formalParameterTypes.IsDefault);
Debug.Assert(!_arguments.IsDefault);
Debug.Assert(_arguments.Length == _formalParameterTypes.Length);

// We expect that we have been handed a list of arguments and a list of the
// formal parameter types they correspond to; all the details about named and
// optional parameters have already been dealt with.

// SPEC: For each of the method arguments Ei:
for (int arg = 0; arg < _arguments.Length; arg++)
for (int arg = 0, length = this.Length; arg < length; arg++)
{
var argument = _arguments[arg];

Expand Down Expand Up @@ -795,7 +800,7 @@ private void MakeOutputTypeInferences(Binder binder, ref HashSet<DiagnosticInfo>
// SPEC: where the output types contain unfixed type parameters but the input
// SPEC: types do not, an output type inference is made from Ei to Ti.

for (int arg = 0; arg < _arguments.Length; arg++)
for (int arg = 0, length = this.Length; arg < length; arg++)
{
var formalType = _formalParameterTypes[arg];
var argument = _arguments[arg];
Expand Down Expand Up @@ -1067,7 +1072,7 @@ private bool DependsDirectlyOn(int iParam, int jParam)
Debug.Assert(IsUnfixed(iParam));
Debug.Assert(IsUnfixed(jParam));

for (int iArg = 0; iArg < _arguments.Length; iArg++)
for (int iArg = 0, length = this.Length; iArg < length; iArg++)
{
var formalParameterType = _formalParameterTypes[iArg];
var argument = _arguments[iArg];
Expand Down
21 changes: 11 additions & 10 deletions src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2407,7 +2407,7 @@ private EffectiveParameters GetEffectiveParametersInNormalForm<TMember>(
{
int parm = argToParamMap.IsDefault ? arg : argToParamMap[arg];
// If this is the __arglist parameter, just skip it.
if (parm == parameters.Length)
if (parm >= parameters.Length)
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me why, if the parameter length and the argument length are mismatched, the only argument beyond the parameter list will be the _arglist parameter.

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 is the only case in non-error code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment explaining why it is important to use >= instead of == here.

{
continue;
}
Expand Down Expand Up @@ -2519,12 +2519,17 @@ internal MemberResolutionResult<TMember> IsMemberApplicableInNormalForm<TMember>
var argumentAnalysis = AnalyzeArguments(member, arguments, isMethodGroupConversion, expanded: false);
if (!argumentAnalysis.IsValid)
{
// When we are producing more complete results, and a required parameter is missing, we push on
// to type inference so that lambda arguments can be bound to their delegate-typed parameters,
// thus improving the API and intellisense experience.
if (!completeResults || argumentAnalysis.Kind != ArgumentAnalysisResultKind.RequiredParameterMissing)
switch (argumentAnalysis.Kind)
{
return new MemberResolutionResult<TMember>(member, leastOverriddenMember, MemberAnalysisResult.ArgumentParameterMismatch(argumentAnalysis));
case ArgumentAnalysisResultKind.RequiredParameterMissing:
case ArgumentAnalysisResultKind.NoCorrespondingParameter:
if (!completeResults) goto default;
// When we are producing more complete results, and we have the wrong number of arguments, we push on
// through type inference so that lambda arguments can be bound to their delegate-typed parameters,
// thus improving the API and intellisense experience.
break;
default:
return new MemberResolutionResult<TMember>(member, leastOverriddenMember, MemberAnalysisResult.ArgumentParameterMismatch(argumentAnalysis));
}
}

Expand Down Expand Up @@ -2824,10 +2829,6 @@ private MemberAnalysisResult IsApplicable(
// treating the method as inapplicable.
paramCount = arguments.Arguments.Count;
}
else
{
Debug.Assert(paramCount == arguments.Arguments.Count);
}

// For each argument in A, the parameter passing mode of the argument (i.e., value, ref, or out) is
// identical to the parameter passing mode of the corresponding parameter, and
Expand Down
57 changes: 57 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/LambdaTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1963,5 +1963,62 @@ public TSource FirstOrDefault(Func<TSource, bool> predicate, params TSource[] de
Assert.Equal("String", typeInfo.Type.Name);
Assert.NotEmpty(typeInfo.Type.GetMembers("Replace"));
}

[Fact]
[WorkItem(557, "https://github.com/dotnet/roslyn/issues/557")]
public void TestLambdaWithError11()
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like we should add tests for scenarios that wouldn't be affected by the change in the parser. For example, when comma is present in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Adding tests without syntax error.

In reply to: 66188319 [](ancestors = 66188319)

{
var source =
@"using System.Linq;

public static class Program
{
public static void Main()
{
var x = new {
X = """".Select(c => c.
Y = 0,
};
}
}
";
var compilation = CreateCompilationWithMscorlibAndSystemCore(source);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to assert errors reported for this scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would be bad to assert the errors produced in this scenario. I think we want to change the parser's handling of these kinds of cases, but this test asserts the things we want to remain invariant about the scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

The errors make it easier to make sense how compiler "understands" the code. Adjusting the baseline, if we change parser behavior, is a one minute change.

In reply to: 66187790 [](ancestors = 66187790)

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer not to add assertions that we neither want to assert nor are the point of the test. While I agree that adjusting the test when the compiler changes is a small task and so not so bad, it is multiplied by the number of times we have burdened ourselves with such unnecessary assertions. Rather than being not so bad, I'd rather not be bad at all.

In reply to: 66187962 [](ancestors = 66187962,66187790)

var tree = compilation.SyntaxTrees[0];
var sm = compilation.GetSemanticModel(tree);
var lambda = tree.GetCompilationUnitRoot().DescendantNodes().OfType<LambdaExpressionSyntax>().Single();
var eReference = lambda.Body.DescendantNodes().OfType<IdentifierNameSyntax>().First();
Assert.Equal("c", eReference.ToString());
var typeInfo = sm.GetTypeInfo(eReference);
Assert.Equal(TypeKind.Struct, typeInfo.Type.TypeKind);
Assert.Equal("Char", typeInfo.Type.Name);
Assert.NotEmpty(typeInfo.Type.GetMembers("IsHighSurrogate")); // check it is the char we know and love
}

[Fact]
[WorkItem(5498, "https://github.com/dotnet/roslyn/issues/5498")]
public void TestLambdaWithError12()
{
var source =
@"using System.Linq;

class Program
{
static void Main(string[] args)
{
var z = args.Select(a => a.
var foo =
Copy link
Member

Choose a reason for hiding this comment

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

Your compiler code is certainly one way to fix this issue (and i still think it's valuable to have). HOwever, i'm pretty surprised that we parse this as having multiple arguments. In the absence of a comma to make us think we can get another argument, and given that "var foo" should be the start of node in a higher parsing context (the block) i would think we would bail out here and just parse this one argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi It is not a "given" that var foo should be the start of a node in a higher parsing context. The compler requires an identifier after a., which must either appear in the input or be inserted by the parser for error recovery. But there is indeed already an identifier var in the input. The simple, non-contextual identifier var is simply the second identifier in the qualified expression a.var. (Note that var is never a contextual identifier - it is always a simple identifier in the syntax) However, in order to make sense of the following foo, the compiler inserts a comma. That is why this invocation has two arguments.

Copy link
Member

Choose a reason for hiding this comment

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

This can be fixed though with a bit of lookahead (and i thought we already did this int he compiler in one place). Basically, instead of blindly consuming 'var' as the name of a., we instead peek ahead. If we see that we have something that would not be valid in the expression context we're in, we ask the following.

  1. Is 'var' on the next line?
  2. Is it followed by whitespace and then an identifier?

If both are true, then we should not consume it as a name. Once we've done that, then parser error recovery comes into play. I would not expect argument parsing to succeed here (absent a comma or other clear indicator that this is an arg, and especially given that it looks like the start of a statement), and i would expect statement parsing to say: "yup, this looks good". We'd then bail out of argument parsing and go into statement parsing.

We did this with ParseVariableDeclarator for example. This is how we make sure htat we parse the following properly:

                // C                    //<-- here
                // Console.WriteLine();

Copy link
Member

Choose a reason for hiding this comment

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

Note: i made this specific parsing fix for typescript which you can see here:

https://github.com/Microsoft/TypeScript/blob/master/src/compiler/parser.ts#L1850

There we look specifically for the pattern:

            // So, we check for the following specific case:
            //
            //      name.
            //      identifierOrKeyword identifierNameOrKeyword

If we see that we do not consume the first 'identifierOrKeyword' as the name of the member access.

Now, in TS this was Especially important as all keywords can be consumed on the right side of a 'dot'. However, it's still a really good heuristic that would improve thing for the 'var' (and other identifier) case for C#.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've opened a new issue for this suggestion. Can you please construct an example that shows how the suggestion would improve the IDE experience, and add it to #11817?

}
}";
var compilation = CreateCompilationWithMscorlibAndSystemCore(source);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to assert errors reported for this scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the same reason, I explicitly avoided doing that.

var tree = compilation.SyntaxTrees[0];
var sm = compilation.GetSemanticModel(tree);
var lambda = tree.GetCompilationUnitRoot().DescendantNodes().OfType<LambdaExpressionSyntax>().Single();
var eReference = lambda.Body.DescendantNodes().OfType<IdentifierNameSyntax>().First();
Assert.Equal("a", eReference.ToString());
var typeInfo = sm.GetTypeInfo(eReference);
Assert.Equal(TypeKind.Class, typeInfo.Type.TypeKind);
Assert.Equal("String", typeInfo.Type.Name);
Assert.NotEmpty(typeInfo.Type.GetMembers("Replace"));
}
}
}