Tighter validation over Binding Class inheritance

[clrudolphi]

🤔 What's changed?

Binding Source Processor is changed to return an Error BindingValidationResult when a binding class inherits from another binding class.

Testing added in RuntimeTests\Binding\Discovery to exercise 30 inheritance combinations.

⚡️ What's your motivation?

To prevent problems as shown in #676

🏷️ What kind of change is this?

• ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

I have identified 30 different scenarios that are combinations of base and derived classes; those with or without the [Binding] attribute, methods that may or may not have Step attributes on them (such as [Given]) and variations that involve derived classes overriding those base class methods that may or may not be step methods.

See the attached for a matrix describing these situations.
Reqnroll Binding Inheritance Situations.xlsx

Each of these combinations is embodied as a test subject class added in this PR (see RuntimeTests\Bindings\Discovery\TestSubjects).

The attached matrix also identifies errors/warnings that Reqnroll does not yet provide, such as a warning when a Step method exists in a class hierarchy that is missing a [Binding] attribute and duplicative binding method registrations (due to class inheritance) that will result in AmbiguousBinding exceptions at run-time.

I invite feedback on how sophisticated Reqnroll should be at identifying these (potential) error conditions during binding discovery. Such checks could be added to this PR or in subsequent work.

📋 Checklist:

• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

---

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

[gasparnagy]

@clrudolphi As far as I understand, in this PR, we focus on the "error" situations only from your excel matrix, right?

Other: I guess this is technically a breaking change, or?

[clrudolphi]

@clrudolphi As far as I understand, in this PR, we focus on the "error" situations only from your excel matrix, right?

I was attempting to identify all the possible binding inheritance possibilities and identify the expected outcomes. The matrix mentions some situations where we (probably) should throw errors or warnings, but for which we don't.

.

Other: I guess this is technically a breaking change, or?

I did change the code to identify and throw errors when binding classes inherit from other binding classes.
As a change that is more restrictive, yes, this would be a breaking change.

[gasparnagy]

A safer approach for this would be the following (background: IBindingType instances that correspond to a .NET type must be RuntimeBindingType).

bool BindingClassInheritsFromBindingClass(BindingSourceType bindingSourceType)
{
    if (bindingSourceType.BindingType is not RuntimeBindingType runtimeBindingType)
        return false;
    var type = runtimeBindingType.Type;
    int count = 0;
    while (type != null && type != typeof(object))
    {
        if (type.IsDefined(typeof(BindingAttribute), false))
        {
            count++;
            if (count > 1)
                return true;
        }
        type = type.BaseType;
    }
    return false;
}

However even with this approach the problem is that the decision about what makes a type "binding type" is duplicated between here and RuntimeBindingRegistryBuilder. Altogether maybe a better approach would be to collect all "recognized" binding types in ProcessType (in a field, like we do for _stepDefinitionBindingBuilders) and scan through all of them in the BuildingCompleted and check if they are among each-others parents.

[clrudolphi]

While I'm working on the refactoring, I'd like clarification of the requirements.
Scenario: Base class has a [Binding] attribute, but has no bound methods. When processed, it is accepted as a binding type. A class that derives from it has bound methods (those with a [Given], etc.), but it does not have a `[Binding] attribute.

The derived class inherits the [Binding] attribute from its base class.

Is this an allowed/supported inheritance structure, or should it have a BindingResult that flags the inheritance?

[Code-Grump]

This really makes me want to deprecate the BindingAttribute type. It's a redundant marker in every situation I can conceive and actively confusing in some like the one illustrated here.

[gasparnagy]

While I'm working on the refactoring, I'd like clarification of the requirements. Scenario: Base class has a [Binding] attribute, but has no bound methods. When processed, it is accepted as a binding type. A class that derives from it has bound methods (those with a [Given], etc.), but it does not have a `[Binding] attribute.

The derived class inherits the [Binding] attribute from its base class.

Is this an allowed/supported inheritance structure, or should it have a BindingResult that flags the inheritance?

I had to check the code history to answer this and what I found was surprising.

To explain, I have to go back in history: Initially the VS extension had to be able to decide based on the code only if something is a binding class or not. On code level (or code AST level) the inheritance is not resolved so it processed the classes that directly have a binding attribute.

So my immediate answer would have been that "inherited" binding attributes are not processed as binding class.

Looking at the runtime code, however, it seems that this is not the case. The code does allow inherited binding attributes as well. I would say that this is a bug.

So following my original intention, the base class with the [Binding] should have a warning (empty binding class) and the derived class with the [Given] is a warning/error: step definition in a non-binding class.

But with the current form of a code (does not matter if it was a result of a bug or what), I am also not fully sure what the expected behavior should be. Maybe we should rather investigate the suggestion of @Code-Grump instead of trying to preserve the current strange behavior with warnings and errors.

This really makes me want to deprecate the BindingAttribute type. It's a redundant marker in every situation I can conceive and actively confusing in some like the one illustrated here.

OK. Let's think it over. So the idea is that whatever class has any members that looks like a binding member (step def, hook, step arg trafo) is implicitly treated as a binding class. I.e. whenever that binding member is "used" (e.g. when a step is invoked that matches to that step def), Reqnroll would ensure an instance of that particular class.

I think this would work for all the obvious valid cases, but I would like us to think about the cases when it might produce a problem. The ones that come to my mind:

• This means we need to process all classes from the test assembly and the registered binding assemblies => I don't think this is a big issue, or?
• If the user had classes without binding but with [Given] - it was ignored so far, now this will be live => not a big problem, but breaking change.
• Any tooling (VSCode, Rider) that was deciding based on the existence of the binding attribute itself needs to be updated => the users of these tools might want to keep the binding attribute until this is fixed. (So we cannot? make it obsolete)
• Any other?