BL0007: Component parameters should be auto property ignores .editoronfig on linux

[linkdotnet]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

Having a component which violates the new BL0007 rule will lead to an build on linux machine (github action) even though the analyzers severity is set to None in a .editorconfig. This seems only to happen on a linux machine.

Also in the latest nightly builds we can reproduce this issue.

Example code from here: https://github.com/bUnit-dev/bUnit/pull/672/commits/54dffbf798da858d5230eb82a3f40f471af6af6a And here the failing build result for that specific commit: https://github.com/bUnit-dev/bUnit/runs/5837646463?check_suite_focus=true

We also could reproduce this with the latest version (7.0.0-preview.3.22175.4 - nightly docker sdk builds) see here: https://github.com/linkdotnet/bUnit/runs/5851324603?check_suite_focus=true#step:6:12

Expected Behavior

Should not be shown as warning / error. Only the following snippet will "remove" the error

#pragma warning disable BL0007
...
#pragma warning restore BL0007

Steps To Reproduce

See bug description

Exceptions (if any)

No response

.NET Version

7.0.100-preview.2.22153.17

Anything else?

No response

[DataJuggler]

I am using Windows and upgrading my components. What broke in .NET 7?

I get this warning if I use VS Preview with .NET 7. I know how to suppress it, but why am I getting it? I do not get this warning in .NET 6. Can this be explained please.

[mikes-gh]

@DataJuggler its new analyser in net7. The problem is still in net6 its just not shown as a warning. The explanation is here https://github.com/dotnet/aspnetcore/issues/26230

[DataJuggler]

Thanks for the explanation. I personally don't like Auto Property with only get; set;, and haven't had any problems after I suppressed the warning.

[linkdotnet]

I do think there are very valid cases, especially two-way binding aka @bind-Value:

[Parameter]
public string Value 
{ 
  get => _value; 
  set
  {
    if (value != _value)
    {
      _value = value;
      ValueChanged.InvokeAsync(value);
    }
  } 
}

[Parameter]
public EventCallback<string> ValueChanged { get; set; }

BL0007 would now suggest moving the ValueChanged.InvokeAsync outside, so I have to duplicate that code where ever that event would have triggered.

[kibblewhite]

Facing the same issue here after moving to .net 7, didn't appear to be a problem on .net 6 - currently using pragma warning disable BL0007 for now. Keeping an eye on this thread to see if any fixes pop up in the future. Thanks team.

[dumiensky]

We are facing the same warnings in multiple components after net7 migration. Is this a bad practice, to have any method fired in the setter? The warning message ('should be auto property') indicates that the use case is misunderstood by the analyzer.

EDIT: Now that i have read the message a couple of times, maybe the intention of it is to indicate, that any component parameter should be an auto property, without side effects?

[Mike-E-angelo]

Updated to net7.0 here and am seeing these warnings as well. In my case I have noticed many calls to the same properties with the same value/references whenever StateHasChanged is called on a parent component so I have a check in there much like this comment to ensure code doesn't execute twice.

[adospace]

@dumiensky I thought the same, at least in Blazor component parameters should be an auto property and any additional logic should be realized inside the OnParametersSet/Async method

[Liero]

Here is why it was added: #26230

@linkdotnet

I do think there are very valid cases, especially two-way binding aka @bind-Value:

[Parameter]
public string Value 
{ 
  get => _value; 
  set
  {
    if (value != _value)
    {
      _value = value;
      ValueChanged.InvokeAsync(value);
    }
  } 
}

[Parameter]
public EventCallback<string> ValueChanged { get; set; }

BL0007 would now suggest moving the ValueChanged.InvokeAsync outside, so I have to duplicate that code where ever that event would have triggered. This is precisely what you shouldn't be doing

It should only invoke the callback when the value is changed by the component, not when the Value is set from outside.

Imagine the nightmare, where InputText calls ValueChanged callback each time new value is provided:

<InputText @bind-Value="_value"  /> - this immediately marks the FieldIdentifier as modified
<InputText @bind-Value="_value" @bind-Value:after="Validate"  /> - this immediately marks the FieldIdentifier as modified
@code 
{    
  string _value = "val";
  Validate(string value)
  {
     _value  =  _value  + "*";  //infinite loop, since this will call ValueChanged in a loop
  }
}

[szalapski]

Ran into this today. I have three cases:

• One Parameter is used as a facade into the "real" backing property which is called from the Parameter's setter
• One parses nulls and empty strings to normalize the user input
• One has getter and setter than exposes a string as an integer though the underlying value is a string. The getter/setter handle the parsing and the cases where that doesn't work.

In fact, this is why we have properties in the first place--to allow abstractions on getting and setting. Otherwise, why not bind to plain old fields?

I like this warning so as to encourage use of @bind:after, but I wonder if there is a more nuanced way.

[UniMichael]

I agree that making it obvious to users that calling EventCallback<T>.InvokeAsync() (or having side-effects) from a property's setter is a bad idea.

However, I'm curious to know if the very common use case of checking if a value is now "dirty" through a custom set method is really something that should be discouraged in the first place.

i.e.

private string? _someParameter;

[Parameter]
public string? SomeParameter
{
    get => _someParameter;
    set
    {
        if (_someParameter != value)
        {
            _updateNeeded = true;
        }
        _someParameter = value;
    }
}

// Now imagine we have 6 other properties that should update that flag when they change.

// ...

protected override async Task OnParametersSetAsync()
{
    // No need for a wall of "if" blocks before the actual thing we care about.
    if (_updateNeeded)
    {
        // ...
    }
}

protected override async Task OnAfterRenderAsync(bool firstRender)
{
    // Maybe we need to run some JavaScript whenever one of the parameters changes.
    if (_updateNeeded)
    {
        // ...
    }
}

[adospace]

@UniMichael What I usually do when authoring components is to have a ghost property for each parameter (at least for each "not-obvious" parameter).

In the OnParameterSet/Async I usually compare the parameter with the ghost value, updating the ghost property if required. The HTML tree is bound to the ghost property.

This lets me put logic in a single place, decide when and if to update the ghost property, etc.

This also has a nice effect, not forcing multiple render passes when the property is updated more than once (not your case, I'm referring to the OP use case).

private string? _internalSomeParameter;

[Parameter]
public string? SomeParameter {get; set;}

protected override async Task OnParametersSetAsync()
{
    if (SomeParameter != _internalSomeParameter)
    {
        // ...
        _internalSomeParameter = SameParameter;
    }
}

Again, I do this only for parameters where a simple auto-property with the HTML direct binding is not enough.

[Mike-E-angelo]

I myself have gotten into the habit of using auto-properties and using Radzen's DidParameterChange in SetParametersAsync. It's simple enough to make your own variation if necessary.

[DataJuggler]

I don't think there is anything wrong setting your value in the setter.

The way I use setters now is for setting parents.

#region Parent
/// <summary>
/// This property gets or sets the value for 'Parent'.
/// </summary>
[Parameter]
public IBlazorComponentParent Parent
{
    get { return parent; }
    set
    {
        // set the value
        parent = value;

        // if the value for HasParent is true
        if (HasParent)
        {
            // Register with the parent
            Parent.Register(this);
        }
    }
}
#endregion

This way controls and pages can talk to child controls and I learned to communicate between pages by making the MainLayout implement IBlazorComponetParent (part of DataJuggler.Blazor.Components). This is useful if you need to send or retrieve information before a page is rendered.

Here is a sample project that registers pages with MainLayout. https://github.com/DataJuggler/NTouch

[UniMichael]

@adospace Yeah, that's kind of what I was trying to avoid with the non-auto parameters, honestly. In cases where any change to the parameters leads to the flag being dirtied, it means I can just collapse the parameter body in my IDE and have a cleaner view of what's actually going on in the lifecycle methods.

I might go with the Raden approach @Mike-E-angelo mentioned (or something similar).

I just hate having the wall of "if value changed" statements in my lifecycle methods.

---

Edit: I guess my main issue with this warning as it stands is that it kind of misleads the developer into thinking Blazor does "magic" behind the scenes with auto-properties, but that's not the case at all (it's just to avoid developers messing up and causing excessive re-renders).

I feel like it should be documented (and made obvious in the docs) and that the warning should be a suggestion instead.