Error recovery when an expression is not provided is very poor

[DoctorKrolic]

VS version: 17.10 Preview 7

Steps to reproduce:

1. Create a sample blazor web app
2. Go to Counter.razor
3. Remove IncrementCount from @onclick handler, resulting in @onclick=""

Expected behavior: I see an single-charachter error on "" similar as how C# would do in such cases:

Actual behavior:

1. Error location takes seemingly random half of the file:
2. There are actually 2 errors: CS1525: Invalid expression term ')' and a razor-specific error RZ2008: Attribute '@onclick' on tag helper element 'button' requires a value. Tag helper bound attributes of type 'Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.Web.MouseEventArgs>' cannot be empty or contain only whitespace. The first one shouldn't be there at all since it is an implementation detail of how razor works. The second one, although, very accurate, seems to be an overkill to me. Having just something like a C# error CS1733: Expression expected would be way easier to read and parse in my head, while would still be pretty accurate - after all, an expression returning a delegate of several allowed shapes is expected to be there.

Additional info: This happens quite a lot in the middle of an edit to a file. And it is pretty annoying to see half of the file become one giant error several times in a row as you type.

[davidwengier]

Personally speaking I've been of the opinion that if the compiler can produce an RZ diagnostic, it shouldn't produce C# code that will also have its own diagnostics. That does affect tooling scenarios though (eg, if there is no C# generated than how do we provide completion when the user invokes it in the attribute value?) so it might not always be possible.

I've labelled this as compiler, but it might be that tooling can make some changes here too, like not reporting the CS diagnostics if they overlap an RZ? Maybe needs a bit of a design process encompassing both teams.

[chsienki]

@davidwengier This definitely an interesting intersection where the implementation details are coming through. I can see the appeal of having the tooling hide the CS errors if they overlap, but that seems weird that tooling and command line would no longer agree on the error list.

My 2c here is that the compiler probably shouldn't be emitting anything in these error cases at all, so there's no incorrect CSharp to be reporting errors on. That being said it's possible that that would break tooling scenarios in those error cases?

Perhaps we could take a roslyn parse tree like approach and emit some sort of 'error' token in those cases? That way the generated chsarp is actually valid for tooling purposes, but the razor error still prevents compilation from succeeding?

[davidwengier]

I was curious so I checked, and currently (lets assume a post-FUSE world so its easier to talk about) given <Component @onclick="" /> the compiler produces the follow. In case you can't see it there is no parameter following this,

__builder.AddComponentParameter(5, "onclick", global::Microsoft.AspNetCore.Components.EventCallback.Factory.Create<global::Microsoft.AspNetCore.Components.Web.MouseEventArgs>(this, ));

My 2c here is that the compiler probably shouldn't be emitting anything in these error cases at all, so there's no incorrect CSharp to be reporting errors on. That being said it's possible that that would break tooling scenarios in those error cases?

In this case if the compiler didn't emit that whole line we'd lose Go To Def and Find All Refs on the attribute name, but we can probably live with that given its an error case anyway.

Perhaps we could take a roslyn parse tree like approach and emit some sort of 'error' token in those cases? That way the generated chsarp is actually valid for tooling purposes, but the razor error still prevents compilation from succeeding?

I think this could work, assuming I am guess what you mean by "error token" 😁. If the compiler had an EmptyValuedAttributeNode or something, then the code-gen for that could be something as simple as nameof(..).ToString();, with appropriate mapping, then we don't lose any tooling. We probably still lose a little bit of tooling because completion for the attribute value itself won't know what type is expected. I can't think of a way to get that back without some kind of IDE filtering of diagnostics.