Skip to content

[Blazor] Consider an analyzer to suggest using foreach instead of for in Blazor code. #16815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
javiercn opened this issue Nov 4, 2019 · 6 comments
Labels
analyzer Indicates an issue which is related to analyzer experience area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone

Comments

@javiercn
Copy link
Member

javiercn commented Nov 4, 2019

Consider an analyzer to suggest using foreach instead of for in Blazor code, as the latter can result in undersired behaviors when the loop variable gets captured inside a lamba:

  • This happens explicitly in event-handlers.
  • This happens implicitly within ChildContent fragments.

We can produce an analyzer that looks at for loops inside generated razor components and produces a warning when the loop variable is being used in an unsafe context.

I agree this is a c# behavior, but specially in the second case it happens implicitly as a result of an implementation detail, which can make it incredibly hard to troubleshoot.

The diagnostic will simply loop at the for loops and see if the loop variable is being used inside the closure.

@Andrzej-W
Copy link

I think it is a very good idea. I have seen similar problems from the beginning of Blazor #15633, #16144, #16073 and many others. Issue #16809 is especially hard to understand if you are not smart enough to look at generated code.

@Andrzej-W
Copy link

Suggestion from analyzer should be "use foreach or local variable ...".

@javiercn javiercn changed the title [Blazor] [Blazor] Consider an analyzer to suggest using foreach instead of for in Blazor code. Nov 4, 2019
@mkArtakMSFT mkArtakMSFT added area-blazor Includes: Blazor, Razor Components analyzer Indicates an issue which is related to analyzer experience enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Nov 4, 2019
@mkArtakMSFT mkArtakMSFT added this to the 5.0.0-preview1 milestone Nov 5, 2019
@YordanYanakiev
Copy link

YordanYanakiev commented Dec 7, 2019

I am strongly against it. Replacing a basic ideology with illogical one because there is a bug is an absurd.
The bug should be fixed one or another way, not replacing the basic ideology of a programmers logic and language property nearly 60 years old.
The fix is as well an easy one - just apply internal/hidden variable as a counter if there is missing such - 2 steps code check I believe will fix this, and probably this won't be the first, nor the last time such fix must be applied.
Such a step should be taken if there is absolutely no way to avoid it.

@Andrzej-W
Copy link

The bug should be fixed

@YordanYanakiev this is not the bug - it is a standard C# behaviour which is simply not known to some people and sometimes it is not obvious if you are not an expert on Blazor internals.

People can write loops with arbitrary complexity and it is not a trivial task to introduce some local variables (sometimes more than one) to automatically solve the problem. Here is a simple example without Blazor stuff for simplicity:

for (int i = 0; i < 10; i++)
{
  WriteLine(i);
  if (i % 2 == 0)
    i++;
  WriteLine(i);
}

Now let's add local variable and replace all references inside loop:

for (int i = 0; i < 10; i++)
{
  int x = i;
  WriteLine(x);
  if (x % 2 == 0)
    x++;
  WriteLine(x);
}

I hope you see immediately that these two loops have different behaviour.

@YordanYanakiev
Copy link

YordanYanakiev commented Dec 8, 2019

Unfortunately I am unable to accept this misconception of the basic ideology of the procedural programming languages. This is illogical and unintuitive. Therefore it should be never part of any kind of programming language.
And Yes - it is a ginormous bug.

@mkArtakMSFT
Copy link
Contributor

We're closing this as this is a c# specific thing, rather than something specific to Blazor.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
analyzer Indicates an issue which is related to analyzer experience area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

4 participants