Roslyn: Obsolete attributes with unexpected argument types can crash the compiler

[RikkiGibson]

Version Used: 441c154

Steps to Reproduce:

using System;

namespace System
{
    public class ObsoleteAttribute : Attribute
    {
        public ObsoleteAttribute(string s1, string s2) { }
    }
}

public class C
{
    [Obsolete("a", "b")]
    public void M()
    {
    }
}

Expected Behavior:
Because the constructor in use does not match any well-known signature, we should produce an obsolete warning with no custom message and a "warning" severity produce no diagnostic at all on usage of C.M.

Actual Behavior:
Crash:

System.AggregateException: One or more errors occurred. (Unable to cast object of type 'System.String' to type 'System.Boolean'.)
 ---> System.InvalidCastException: Unable to cast object of type 'System.String' to type 'System.Boolean'.
   at Microsoft.CodeAnalysis.AttributeData.DecodeObsoleteAttribute() in /_/src/Compilers/Core/Portable/Symbols/Attributes/CommonAttributeData.cs:line 283
   at Microsoft.CodeAnalysis.AttributeData.DecodeObsoleteAttribute(ObsoleteAttributeKind kind) in /_/src/Compilers/Core/Portable/Symbols/Attributes/CommonAttributeData.cs:line 252
   at Microsoft.CodeAnalysis.CSharp.Symbol.EarlyDecodeDeprecatedOrExperimentalOrObsoleteAttribute(EarlyDecodeWellKnownAttributeArguments`4& arguments, CSharpAttributeData& attributeData, ObsoleteAttributeData& obsoleteData) in /_/src/Compilers/CSharp/Portable/Symbols/Symbol_Attributes.cs:line 186
   at Microsoft.CodeAnalysis.CSharp.Symbols.SourceMethodSymbolWithAttributes.EarlyDecodeWellKnownAttribute(EarlyDecodeWellKnownAttributeArguments`4& arguments) in /_/src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs:line 340
   at Microsoft.CodeAnalysis.CSharp.Symbol.EarlyDecodeWellKnownAttributes(ImmutableArray`1 binders, ImmutableArray`1 boundAttributeTypes, ImmutableArray`1 attributesToBind, AttributeLocation symbolPart, CSharpAttributeData[] boundAttributesBuilder) in /_/src/Compilers/CSharp/Portable/Symbols/Symbol_Attributes.cs:line 579
   at Microsoft.CodeAnalysis.CSharp.Symbol.LoadAndValidateAttributes(OneOrMany`1 attributesSyntaxLists, CustomAttributesBag`1& lazyCustomAttributesBag, AttributeLocation symbolPart, Boolean earlyDecodingOnly, Binder binderOpt, Func`2 attributeMatchesOpt) in /_/src/Compilers/CSharp/Portable/Symbols/Symbol_Attributes.cs:line 362
   at Microsoft.CodeAnalysis.CSharp.Symbols.SourceMethodSymbolWithAttributes.GetAttributesBag(CustomAttributesBag`1& lazyCustomAttributesBag, Boolean forReturnType) in /_/src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs:line 279
   at Microsoft.CodeAnalysis.CSharp.Symbols.SourceMethodSymbolWithAttributes.GetAttributesBag() in /_/src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs:line 229
   at Microsoft.CodeAnalysis.CSharp.Symbols.SourceMethodSymbolWithAttributes.GetAttributes() in /_/src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs:line 293

We assume that an ObsoleteAttribute constructor with a sufficient number of parameters will have certain types for those parameters.

https://github.com/dotnet/roslyn/blob/1490d43ed98e45d67fbaa60b6a3f2f31cc01885e/src/Compilers/Core/Portable/Symbols/Attributes/CommonAttributeData.cs#L291

/cc @AlekseyTs

[AlekseyTs]

Because the constructor in use does not match any well-known signature, we should produce an obsolete warning with no custom message and a "warning" severity.

Due to outlined reason we should ignore the attribute. All well-known attributes are supposed to be ignored if we do not recognize constructor's signature

[RikkiGibson]

You're saying we should produce no diagnostic at all?

[AlekseyTs]

You're saying we should produce no diagnostic at all?

Yes, as if there was no attribute. We don't recognize constructor, we do not know what it means

[terrajobst]

@RikkiGibson how did you find this? By accident or because of my feature request? My feature deliberately didn't add any constructors to ObsoleteAttribute, hence my question to avoid any confusion.

[RikkiGibson]

@terrajobst found in the course of testing the additions to ObsoleteAttribute. Suspected a bug when reviewing the existing code that gathers information from a usage of ObsoleteAttribute.

The compiler implementation of the new feature is still built around looking for well-known properties on the attribute, as was specified, not looking for any new well-known constructors.

[AlekseyTs]

The problem is probably not specific to the Obsolete attribute, likely it is affecting all well-known attributes. If I remember correctly, we had a stronger signature check at one point, but that probably got relaxed due to some circularity issue in some scenario for some attribute and we never got back to tighten the check or otherwise make the attribute cracking code robust to malformed declarations.

[AlekseyTs]

Another situation that we should handle more gracefully is when an enum type used as argument has incorrect underlying type. For example, this program causes a crash because underlying type of the enum in uint instead of int:

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Numerics;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Text.RegularExpressions;

namespace Windows.Foundation.Metadata
{
    [AttributeUsage(AttributeTargets.Delegate | AttributeTargets.Enum | AttributeTargets.Event | AttributeTargets.Field | AttributeTargets.Interface | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Class | AttributeTargets.Struct, AllowMultiple = true)]
    public sealed class DeprecatedAttribute: Attribute
    {
        public DeprecatedAttribute(string message, DeprecationType type, uint version){}
        public DeprecatedAttribute(string message, DeprecationType type, uint version, Platform platform){}
        public DeprecatedAttribute(string message, DeprecationType type, uint version, string contract){}
    }
    public enum Platform : uint
    {
        Windows = 0,
        WindowsPhone = 0x1,
    }
    public enum DeprecationType : uint
    {
        Deprecate = 0,
        Remove = 0x1,
    }
}

[Obsolete]
class C{}

[Windows.Foundation.Metadata.Deprecated("Test message", Windows.Foundation.Metadata.DeprecationType.Deprecate, 2, Windows.Foundation.Metadata.Platform.Windows)]
class Class
{

}

public class Program
{
    static void Main(string[] args)
    {
        var c = new C();
        var cls = new Class();
    }
}