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

Version Used: 16.11-preview, .net6-preview4

Steps to Reproduce:

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>

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
        }
    }
}

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.
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.

About this issue

Original URL
State: open
Created 3 years ago
Reactions: 2
Comments: 48 (32 by maintainers)

Most upvoted comments

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.

sharwell on Jul 7, 2021

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

dotMorten on Jun 30, 2021

@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.

jcouv on Jun 18, 2021

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! 😃

CyrusNajmabadi on Jun 17, 2021

@CyrusNajmabadi Here’s an example: there are several members in EditorFeatures.Wpf that compile for net472 and netcoreapp3.1, but don’t compile for netstandard2.0 due to implementation constraints. Roslyn could provide a netstandard2.0 reference assembly containing only the signatures valid in both targets, even though we could never provide an implementation assembly for netstandard2.0. Rather than #if the contents of every single method with errors in the reference assembly build, we just ignore them and the compiler will too.

sharwell on Jun 17, 2021

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.

terrajobst on Jun 16, 2021

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.

dotMorten on Jun 16, 2021