IDE shows warnings for invalid code that isn't bound in a ref-only build

[dotMorten]

Version Used:
16.11-preview, .net6-preview4

Steps to Reproduce:

1. Create the following project:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFrameworks>netstandard2.0;net6.0</TargetFrameworks>
  </PropertyGroup>
  <PropertyGroup Condition="'$(TargetFramework)'=='netstandard2.0'">
    <ProduceOnlyReferenceAssembly>True</ProduceOnlyReferenceAssembly>
    <ProduceReferenceAssembly>False</ProduceReferenceAssembly>
  </PropertyGroup>
</Project>

2. Add the following class:

using System;
using System.Threading.Tasks;

namespace MyLib
{
    public class TestClass
    {
        public Task<int> ImportantMethod()
        {
#if NETSTANDARD2_0
            if (PropertyNotDeclared)
	            return;
#else
            return Task.FromResult(0);
#endif
        }
    }
}

3. Compile the project. Notice that there are no build errors, even though the netstandard target references a property that doesn't exist. This is expected as it's aref-only build, and members aren't bound.
4. Notice that the IDE does show that there are errors:

Expected Behavior:
Not sure. Is the IDE wrong?
See long discussion here: https://twitter.com/JonathonMarolf/status/1405046803477667846

There are arguments for why this is correct behavior, but the squiggles showing an error, while things compile fine confused me a lot. In my case, not having to implement stubs for the netstandard build is actually a really nice benefit and saves me a lot of time, but the IDE showing errors is very confusing to the developer. If it is considered correct that the IDE is showing these "errors" even when the compilation is fine, perhaps consider a project setting that will make the IDE allow this and ignore these errors for the cases where we want to take advantage of not needing implementation for a specific target.

Here's a more real-world example of the above class (in my case it's a lot larger implementation of classes required for stubbing, or if-def every single method calling into a native implementation).

using System;
using System.Threading.Tasks;

namespace MyLib
{
    public class TestClass
    {
        public int SomeMethod()
        {
           return Method();
        }
#if __IOS__
       public int Method() { /*ios implementation*/ return 0; }
#elif __ANDROID__
       public int Method() { /*android implementation*/ return 1; }
#elif WINDOWS
       public int Method() { /*windows implementation*/ return 2; }
#endif
    // No need to create fake implementation for netstandard
    }
}

In my specific case I'm creating .NET MAUI handlers and wrapping native UI controls across several platforms that don't exist in the netstandard build, and would never actually be used at runtime. It is really nice that I don't have to either litter all my code with if-def or waste time writing .net standard stubs that'll not even get compiled/used; just so I can satisfy the IDE and not confuse developers editing the code.

[dotnet-issue-labeler]

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.

[terrajobst]

What should the IDE behavior be? Should all method bodies be considered unreachable and thus faded out with no binding?

[dotMorten]

@terrajobst If the decision is that the IDE shouldn't be showing squiggles (either by default or by toggle), I do like the idea that the member contents gets greyed out if it's inactive code. However, if you put invalid syntax in the method body (for example { not matching } or a missing ; compilation would still fail, and squiggles should be shown for that, so should that bit be greyed out?

[terrajobst]

Generally speaking, unreachable code is still parsed/bound. The problem with not binding method bodies is that you won't get any tooling support (code completion, parameter info, quick info etc). One could argue that this would be OK because the developer gets to choose which target framework they want to get IntelliSense for (via the drop down in the top left corner of the editor). For target frameworks that have <ProduceOnlyReferenceAssembly> maybe the answer would be: don't bind method bodies because it's turned off and developers aren't expected to edit them.

[dotMorten]

I guess another option is to both show squiggles AND grey out the code? (and perhaps turn the level of red down a notch)

[CyrusNajmabadi]

What errors are you getting?

[davidwengier]

Personally, I think the errors are appropriate. The code you're looking at is not correct, and what is the IDEs job if not to show you code?
If you really don't care about what is inside the method bodies then just put throw null; everywhere. If you do care about what is inside the method bodies, then the IDE should be helping you get it correct.

[dotMorten]

If you really don't care about what is inside the method bodies then just put throw null; everywhere.

The point is I shouldn't really need to waste my time doing that everywhere in the ref-only target. I mean this is a whole lot cleaner:

    public void MyMethod() => MethodNotInRefOnlyBuild();

Than this:

    public void MyMethod()
    {
#if REF_ONLY
        throw null;
#else
       MethodNotInRefOnlyBuild();
#endif
    }

Now multiply that by the 30 methods I have just in one of my classes and it gets ugly. The other alternative is to declare MethodNotInRefOnlyBuild and all the other methods I'm wrapping, but that is just as ugly, and again I'm writing code that will never even be in the build, so why bother? Having to do it just so you don't get squiggles that'll confuse any developer working with the source code isn't a really good reason, but to be honest I'd rather do that, then look at all those red confusing lines. So my argument is: Let's not confuse developers while not forcing developers to write code that isn't needed.

[terrajobst]

Indeed. Logically, specifying <ProduceOnlyReferenceAssembly> is the same as saying this:

public void MyMethod()
{
#if !REF_ONLY
       MethodNotInRefOnlyBuild();
#endif
}

And for code that isn't active, the IDE reports not errors either.

[CyrusNajmabadi]

Can someone explain what errors are being reported?

[CyrusNajmabadi]

And for code that isn't active, the IDE reports not errors either.

So the ide doesn't generate errors. It just reports the errors that the compiler produces. So we need to know what errors are being produced here. If that information can be provided, that would be helpful. Thanks!

[dotMorten]

@CyrusNajmabadi The main question is more whether errors that doesn't happen at compile time should be reported at all. The errors are sort of in a way valid, except it is occuring in code that is never used. So the specific error (and there are multiple) isn't as important as the IDE behavior, however the repro is above.

[CyrusNajmabadi]

I literally just want to know waht errors are being reported :) It is relevant to me as i'm trying to figure out waht subsystem is reporting the issue.

I literallyu do not have enough information to make a call here as i cannot really understand what is going on. Can you please provide the requested information as it will help be understand a lot better what is happening here. Thanks!

[dotMorten]

@CyrusNajmabadi got it. I'll get that to you in a bit

[sharwell]

@CyrusNajmabadi from the twitter thread:

[CyrusNajmabadi]

Thanks!

I'm trying to understand the goal here from teh end author. Why was the body supplied with:

if (LELPROPDECLARED)
    return;

I'm legit just trying to understand why that would be written and why it would be in that sort of form. i.e. it's C# code... but isn't following the rules of the language. What was the reason for adding that?

[dotMorten]

I'm not sure what that code is. I think I made my case for this in the initial description of the issue with some more realistic code.

[CyrusNajmabadi]

I think I made my case for this in the initial description of the issue with some more realistic code.

In your 'more realistic code' what errors are you getting? Thanks!

[dotMorten]

@CyrusNajmabadi These only show when you select "Build + Intellisense":

[CyrusNajmabadi]

Ok. So my question remains:

I'm legit just trying to understand why that would be written and why it would be in that sort of form. i.e. it's C# code... but isn't following the rules of the language. What was the reason for adding that?

Basically... what is the reason for writing if (PropertyNotDeclared). Why would it be desirable to have such code in the method ever? What is a maintainer supposed to make of this? Similarly, what is the goal of having a naked return; in a non-void method here? What was this code trying to say? Why would someone write this?

I'm genuinely coming up blank what the end customer scenario is here and why this would be something to try to support.

I've also seen teh larger example, but i have an unanswered question on that here: #54141 (comment)

[dotMorten]

@CyrusNajmabadi That specific code was to just repro the issue the simplest way possible. See my second example in the issue description.

[CyrusNajmabadi]

This is only occurring in the IDE.

I get that. But the IDE itself is many components interoperating (including calling into the compiler). This information is really helpful to get an immediate gauge on which of the components are likely having a problem and eliminate those that are likely not involved. Thanks! :)

[jmarolf]

Adding @jaredpar's original diagnosis from twitter:

It's a bit of a gray area but given how the API models elements I lean towards it being "By Design".
Look one layer deeper though. Imagine the following:

void M() {
  DoesNotExist e; 
} 

Now imagine you run GetSemanticModel().GetSymbolInfo().Type on the local. What does that return? Has to be an error type but how can you have an error without diagnostics to describe it?
It's a bit of a mental conflict. The compiler isn't going to realize any diagnostics when binding a method because we don't bind it. The API though gives you the tools to force the compiler to bind it. Hence the IDE gets to decide which it should be displaying.
At an API level I think you pretty much have to surface the diagnostics. The other option is to surface error types and expressions but provide no information as to why there are errors. It would be explicitly hiding information.
The IDE though could make a decision to not surface these. Basically "it's ref only, these won't be emitted, just hide them". Think that would lead to other problems though.
For instance u expect some features won't work in the face of errors (think complex refactorings). If u see the errors and the operations don't work there are clues why. On the other hand if the errors are all suppressed then you can be left wondering why.
On the whole I think the overall better decision is surface the errors. It has less surprising downsides. Also the case which has the wrong experience is the minority case. Most refonly builds will be diagonstic free so why optimize for it at the expense of other scenarios?
The compiler is not surfacing the errors. That is very different than the errors not existing.
The design here is specifically to be tolerant of errors. The errors still exist, some are called out in the design explicitly. They are just ignored for the purpose of emitting the ref only assembly. It doesn't change that they are still present in the code / API.

@CyrusNajmabadi As I understand it: all semantic errors are not emitted by the compiler (at the commandline) if they are inside of a method body and that method body's contents are being elided as part of generating a reference assembly. While @jaredpar labels this as "By Design" I believe the operserved error behavior is more of an implementation detaul. The compiler never attempts to bind the method body so at the commandline no binding errors are reported.

This is what is happening from the compiler's perspective (approximately):

using System;

class Program
{
    static void M()
    {
#if SOME_UNDEFINED_SYMBOL
        Console.WriteLine("Hello World!");
#endif
    }
}

So if I am writing some code that I expect to compile but I accidentally added a binding error (say I mis spell Console.WriteLine as Console.WirteLine) I will not get diagnostics at the commandline. Everything will build successfully on the commandline. But I will see errors in Visual Studio.

using System;

public class C
{
    public void M()
    {
#if NETSTANDARD2_0
        Console.WirteLine("Hello World");
#else
        Console.WriteLine("Hello World");
#endif
    }
}

[CyrusNajmabadi]

We legit just ask the compiler for diagnostics. We have no idea about this flag or what it would mean. It's not even clear what the ide could do here being trying to I've 8 the compiler's rules for what it does here. But that goes against our philosophy of not reimplementing compiler logic. :-)

[jaredpar]

I agree with not re-implementing compiler logic. I had thought a bit about whether or not we should be showing or hiding these diagnostics. The problem with hiding them is that it creates other strange behaviors. For example consider the following:

public class C { 
  public void M() {
    DoesNotExist e;
  }
}

Lets say that the compiler chooses to suppress all diagnostics in refonly mode. Now if you use the SemanticModel to inspect the local symbol here you are going to get an error symbol but no diagnostics explaining why you have an error symbol. That seems decently incorrect here

Under the hood these errors do exist, it's just that the compiler is choosing not to surface them in refonly. Them being displayed in the IDE isn't a contradiction here.

[jmarolf]

Assumptions

1. We want diagnostics in the IDE and at the command line to be the same. It's confusing if I get errors in the IDE that are not there when I build.
2. The purpose of diagnostics are to tell you if a program is "correct" (this has changed over time but with the inclusion of SDK analyzers we cannot say that we only want to show diagnostics for "something is a legal C# program"). While correct is a bit of a fuzzy word to use here I interpret this as operating according to the principle of least surprise.

Background

1. Reference assemblies were designed to solve the hard problem of stripping out method bodies from an assembly. There are many corner-cases here that are hard to get correct, so this work was moved to the compiler layer as they were best equipped to do it correctly. (see detailed doc here)
2. This is a bit of a behavioral gray area. Reference assemblies are not part of the C# language specification, so their behavior is an implementation detail.
3. There are already places today where the IDE needs to call roslyn APIs to surface diagnostics as the compiler does not do this on its own (see Have compiler return semantic errors in incomplete members. #7536). We would like to avoid this entirely as we do not want the diagnostics the IDE shows to be different from the commandline but we are not there yet.
4. Even if the compiler does not choose to report a diagnostic it can be made to. An analyzer calling an API can force things like binding to happen which will cause the diagnostics to be produced. This detail has been know as far back as when determinism was first implemented in the compiler. It was pointed out that the compiler cannot be deterministic with analyzers because even if we suppress all diagnostics from the analyzers themselves simply letting them run could affect the compiler diagnostic output.

Proposal

After talking with @jcouv I am amending this proposal
I think the compiler should report diagnostics here. I feel that the design decision to not produce binding diagnostics makes the feature more complex for developers to understand and use. Yes, all that code that is being bound is not going to be emitted but I would argue the developer wants to know if something they have written is wrong even if things could workout at runtime.

From the IDE perspective we should treat command-line options that emit a ref assembly as putting us in a special mode where the compiler does not produce diagnostics if a program is invalid.

In this particular case, we should gray out everything in the NETSTANDARD2_0 to indicate to users that it is not going to be emitted and that is why no diagnostics are being reported

using System;

public class C
{
    public void M()
    {
#if NETSTANDARD2_0
        Console.WirteLine("Hello World");
#else
        Console.WriteLine("Hello World");
#endif
    }
}

I would also like a compiler API to let us know that a method body is not going to be emitted because (as @CyrusNajmabadi points out) we do not what to re-implement all the logic about how to strip method bodies the compiler has already done.

Related Conversions

• [Work items] Refining ref assemblies #17612 (comment)
• Further refinements on ref assemblies #19994
• Produce skeleton assemblies tolerating errors #20914
• Make the command-line compilers able to generate reference assemblies #2184

@jcouv I couldn't find the original discussion on where we decided to not emit binding diagnostics, do you remember?

[jaredpar]

I think the compiler should report diagnostics here. I feel that the design decision to not produce binding diagnostics makes the feature more complex for developers to understand and use. Yes, all that code that is being bound is not going to be emitted but I would argue the developer wants to know if something they have written is wrong even if things could workout at runtime.

The design here is to specifically allow ref assembly generation in the face of errors. That opens up a number of other scenarios. Flipping that decision would remove those scenarios, reduce throughput and be a back compat break. Low chance of us making that change.

I would also like a compiler API to let us know that a method body is not going to be emitted because (as @CyrusNajmabadi points out) we do not what to re-implement all the logic about how to strip method bodies the compiler has already done.

This decision exists outside the standard SemanticModel API because it's an emit time decision. Any place where we have access to EmitOptions it's easy to detect this because that is where the decision resides. Lacking EmitOptions there is no real context to know whether or not method bodies are emitted.

The IDE though should have access to this when building up the initial workspace. That, in theory at least, has access to the entire command line which will expose EmitOptions.

[dotMorten]

Both me and @sharwell did describe a scenario where we don't even want to have a .net standard implementation and just need ref assemblies for that.
I think it feels rather silly forcing me to litter my code with #if conditions or create mock classes for my ref build, just to satisfy intellisense. It's a waste of my time, and more code to maintain, that really serves no purpose.

I'm not fully understanding the arguments above - they seem to me to be more technical than practical, but I'll admit I'm not fully grasping that part. However in that case it seems to me that there are two use-cases, and the above conclusions only account for one.

Perhaps an option is to allow us to "disable" these warnings for ref-only as an opt-in at the project level?

[jmarolf]

The design here is to specifically allow ref assembly generation in the face of errors. That opens up a number of other scenarios.

@jaredpar this is a wild idea but what if all binding errors inside of a reference-assembly method were just warnings? That feels like the point of warnings: "Its weird that this is happening, but you can continue." We used to call this feature "Error-as-Warning" anyways :)

The IDE though should have access to this when building up the initial workspace. That, in theory at least, has access to the entire command line which will expose EmitOptions.

Great, if we have the APIs necessary to do this in the short term. We obviously have the compilation options and emit options If someone could detail a sample of what the correct api calls would look like that would be ideal. Still dream of a day when the compiler is in charge of diagnostics full-stop and things like #7536 are resolved.

[jcouv]

@jmarolf I don't agree with the proposed behavior. I would rather aim to show the same diagnostics in the IDE as we do in the command-line build (ie. none for this project).
As for why that's not working that way already, I assume the compilation created by the IDE isn't given the /refonly option from the command-line build. But I haven't debugged to confirm that yet.
Let's find some time to chat tomorrow/Friday.

[Update:] From chat with Jon, here's my understanding: the IDE runs some analyzers that are not part of the command-line compilation. Such analyzers may call semantic model APIs which cause method bodies to be bound, and this has a side-effect of adding diagnostics to the compilation the IDE uses for the Error List. But we agree that the IDE and the command-line should preferably display the same diagnostics.

There's no easy way to fix this:

• avoiding IDE-specific analyzers: I don't think that's very realistic
• having the compiler APIs refrain from adding diagnostics that pertain to binding method bodies: this is doable, but requires reviewing every place where a diagnostic is added and consider the emit option.

In the short-term, I would recommend to /refonly users to either
(1) use the same code for /refonly project that they use for their other project (no #if NETSTANDARD2_0 in this case), or
(2) if that causes some problems, use throw null; as the ignored "implementation" for the /refonly project.

[sharwell]

The command line compiler should not report diagnostics of any kind for semantic errors inside of method bodies when /refonly is specified. Reporting these diagnostics requires full binding for method bodies, which is one of the most expensive portions of compilation. We should not force a performance penalty into the scenario when the vast majority of uses of /refonly are automation scenarios.

[sharwell]

Design review conclusion: The IDE should pass through the same options to the compiler that would be used in a command line build. A follow-up issue should be filed to separately implement an analyzer that grays out all code inside ref-only implementation methods as unnecessary code, to avoid having users think the code is being checked for semantic accuracy.

[dotMorten]

@sharwell Thanks Sam! I think this sounds like a great decision

[terrajobst]

[CyrusNajmabadi]

Closing out due to lack of movement.

[terrajobst]

I'm confused why this was closed without further explanation. It seems the design review concluded that an action should be taken. Is that not true?

[CyrusNajmabadi]

Primarily because this was assigned to no one with no milestone. This was effectively never going to get done. It read closed to reflect that that's the case.

If this is something that a partner team needs, please let us know, including when you need it by. We can then assign to a dev and milestone for the work to actually happen.