WTG3007 should allow overrides that include sealed keyword

[andrewhongnsw]

In the following code, WTG3007 triggers at each of the four overrides - however sealing is arguably not pointless as it prevents further overrides.

Recommend checking for the sealed keyword and allowing overrides in those cases.

    class DerivedClass : BaseClass
    {
        public sealed override void Method(int argument1) => base.Method(argument1);

        public sealed override object Property
        {
            get => base.Property;
            set => base.Property = value;
        }

        public sealed override event EventHandler Event
        {
            add => base.Event += value;
            remove => base.Event -= value;
        }

        public sealed override object this[int index]
        {
            get => base[index];
            set => base[index] = value;
        }
    }

    class BaseClass
    {
        public virtual void Method(int argument1)
        {
        }

        public virtual object Property { get; set; }

        public virtual event EventHandler Event
        {
            add { }
            remove { }
        }

        public virtual object this[int index]
        {
            get => null;
            set { }
        }
    }

[andrewhongnsw]

My suggestion is something like this in PointlessOverrideAnalyzer.cs:

if (IsOverride(node) && !DeclarationHasKeyword(node, SyntaxKind.SealedKeyword) && !node.Accept(IsMeaningfulVisitor.Instance))

and

static bool DeclarationHasKeyword(CSharpSyntaxNode node, SyntaxKind keyword)
{
    return node switch
    {
        MethodDeclarationSyntax => ((MethodDeclarationSyntax)node).Modifiers.Any(x => x.IsKind(keyword)),
        PropertyDeclarationSyntax => ((PropertyDeclarationSyntax)node).Modifiers.Any(x => x.IsKind(keyword)),
        IndexerDeclarationSyntax => ((IndexerDeclarationSyntax)node).Modifiers.Any(x => x.IsKind(keyword)),
        EventDeclarationSyntax => ((EventDeclarationSyntax)node).Modifiers.Any(x => x.IsKind(keyword)),
        _ => false,
    };
}

[brian-reichle]

I agree that it should not be considered pointless if it's sealed, though I wouldn't implement the check like that. I would suggest using an approach similar to what IsOverride does; in fact, it might be a good idea to merge the two.

bool IsApplicable(CSharpSyntaxNode node)
{
    var modifiers = node.Accept(ModifierExtractionVisitor.Instance);
    return HasToken(modifiers, SyntaxKind.OverrideKeyword) && !HasToken(modifiers, SyntaxKind.Sealed);
}

static bool HasToken(SyntaxTokenList list, SyntaxKind kind)
{
    foreach (var token in list)
    {
        if (token.IsKind(kind))
        {
            return true;
        }
    }

    return false;
}

or perhaps

bool IsApplicable(CSharpSyntaxNode node)
{
    var modifiers = node.Accept(ModifierExtractionVisitor.Instance);
    var hasOverride = false;

    foreach (var token in modifiers)
    {
        switch (token.Kind())
        {
            case SyntaxKind.SealedKeyword:
            case SyntaxKind.VirtualKeyword:
                return false;
            case SyntaxKind.OverrideKeyword:
                hasOverride = true;
                break;
        }
    }

    return hasOverride;
}

[yaakov-h]

Created WI00577893 to track this internally.

[brian-reichle]

I had already created WI00577538 :)

[yaakov-h]

lol ok, cancelled WI00577893