dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.12k stars 4.04k forks source link

VB: ConditionalAttribute is not honoured when applied to interface methods. #22086

Open ericmutta opened 7 years ago

ericmutta commented 7 years ago

Version Used:

Visual Studio Community 2017 v15.3.3

Steps to Reproduce:

Type in the following code in VB console project:

Public Module Module1
  Public Interface ITracer
    <Conditional("DEBUG")>
    Sub ThisHappened()

    <Conditional("DEBUG")>
    Sub ThatHappened()
  End Interface

  Public Class CTracer
    Implements ITracer

    Public Sub ThisHappened() Implements ITracer.ThisHappened
      Console.WriteLine("THIS happened.")
    End Sub

    Public Sub ThatHappened() Implements ITracer.ThatHappened
      Console.WriteLine("THAT happened.")
    End Sub
  End Class

  Public Sub Main()
#If DEBUG Then
    Console.WriteLine("DEBUG BUILD: you should see messages below.")
#Else
    Console.WriteLine("RELEASE BUILD: you should only see this line.")
#End If

    Dim tracer As ITracer = New CTracer()

    tracer.ThisHappened() '<-- should not be compiled to MSIL in release build.
    tracer.ThatHappened() '<-- should not be compiled to MSIL in release build.

    Console.ReadLine()
  End Sub
End Module

If you run the above code in DEBUG build, it prints the following as expected:

DEBUG BUILD: you should see messages below.
THIS happened.
THAT happened.

If you run the code in RELEASE build, the compiler does not honour the ConditionalAttribute applied to the interface methods ThisHappened and ThatHappened so the program mistakenly produces this output:

RELEASE BUILD: you should only see this line.
THIS happened.
THAT happened.

Expected Behavior:

The MSDN ConditionalAttribute documentation says that it:

Indicates to compilers that a method call or attribute should be ignored unless a specified conditional compilation symbol is defined.

However the VB compiler doesn't honour that for these two method calls done via an interface variable:

    Dim tracer As ITracer = New CTracer()

    tracer.ThisHappened() '<-- should not be compiled to MSIL in release build.
    tracer.ThatHappened() '<-- should not be compiled to MSIL in release build.

Unless I have overlooked some (undocumented?) technical reason, I believe this is a bug.

gafter commented 7 years ago

The C# language makes it an error to apply ConditionalAttribute to an interface method. VB permits it but ignores it. I recommend we either respect the attribute, or warn about the application of the attribute in an interface. It is probably too late to make it an error.

ericmutta commented 7 years ago

@gafter The C# language makes it an error to apply ConditionalAttribute to an interface method.

Just out of curiosity...why is the attribute not honoured/allowed? Is there a technical reason?

gafter commented 7 years ago

@ericmutta No idea.

ericmutta commented 7 years ago

@gafter that's good news then, it means VB can go ahead and honour it since it already permits its application on interface methods 👍

For some context on why someone might want this (see code I gave earlier above):

I have a tracing interface used to write debugging information. In unattended test sessions I want tracing output to go to a file. In interactive test sessions I want tracing output to go to the console. So I had different trace recorders implement that interface. So far so good. When I switch to release build, I want the tracer interface calls to be removed completely (but that didn't happen despite the Conditional("DEBUG") attribute - hence the bug report).