bad linq transformation

On a coworker's install, he tried to use the "convert to linq" quick fix,
and the result was clearly broken code (wouldn't even compile):

Before:
            var numRows = 0;
            foreach (var column in group.Columns)
            {
                if (column.Fields.Count > numRows)
                    numRows = column.Fields.Count;
            }


After:
            var numRows = 0;
            int rows = numRows;
            foreach (var column in group.Columns.Where(column =>
column.Fields.Count > rows))
            {
                numRows = column.Fields.Count;
            }


Clearly the 'before' code is attempting to find the maximum number of rows
in a set... yet after performing the quick-fix, it gets the wrong value.
Further, there are other warnings or quick fixes offered that, if selected,
result in increasingly broken code.

Is this a known bug or limitation?  Anyone else experience anything like
this?  I can get you more details if you need... but this was completely
reproducible on his machine.



6 comments
Comment actions Permalink

I think this is a limitation in this case. The quick fix is not smart enough to know what you really wanted.

group.Columns.Select(c=>c.Fields.Count).Max() is what you need.

0
Comment actions Permalink

Hello Paul,

Thank you very much for reporting this problem! I've filed a bug-report in
our issue tracker: http://youtrack.jetbrains.net/issue/RSRP-191007 and you're
welcome to monior its status.

Andrey Serebryansky
Support Engineer
JetBrains, Inc
http://www.jetbrains.com
"Develop with pleasure!"

var numRows = 0;
foreach (var column in group.Columns)
{
if (column.Fields.Count > numRows)
numRows = column.Fields.Count;
}
After:
var numRows = 0;
int rows = numRows;
foreach (var column in group.Columns.Where(column =>
column.Fields.Count > rows))
{
numRows = column.Fields.Count;
}



0
Comment actions Permalink

Yes, that is really what is desired... but ReSharper's quick-fixes shouldn't
break code.  And this clearly does.

Worse, the code it creates has issues (such as referencing a loop variable
in a closure), and each attempt to fix those problems with quick fixes
breaks the code even worse, until it doesn't even compile.  It's a serious
problem that shakes my blind faith in using quick-fixes safely.

If it can't successfully transform the code without altering behavior or
introducing new problems, it shouldn't offer to transform the code IMHO.



"Sven Heitmann" <no_reply@jetbrains.com> wrote in message
news:11600833.61091284465984478.JavaMail.devnet@domU-12-31-39-18-36-57.compute-1.internal...

I think this is a limitation in this case. The quick fix is not smart
enough to know what you really wanted.

>

group.Columns.Select(column.Fields.Count).Max() is what you need.

>

---
Original message URL: http://devnet.jetbrains.net/message/5272317#5272317


0
Comment actions Permalink

Thanks!  This one is pretty serious, imho... any chance for a future 5.x
update that might contain a fix for this (and related) issues?  Having
confidence that quick-fixes won't break code or result in uncompilable code
is pretty important.

"Andrew Serebryansky" <andrew.serebryansky@jetbrains.com> wrote in message
news:c8a898dd9c7e8cd23d43f09a0b1@news.intellij.net...

Hello Paul,

>

Thank you very much for reporting this problem! I've filed a bug-report in
our issue tracker: http://youtrack.jetbrains.net/issue/RSRP-191007 and
you're welcome to monior its status.

>

Andrey Serebryansky
Support Engineer
JetBrains, Inc
http://www.jetbrains.com
"Develop with pleasure!"

>
>> var numRows = 0;
>> foreach (var column in group.Columns)
>> {
>> if (column.Fields.Count > numRows)
>> numRows = column.Fields.Count;
>> }
>> After:
>> var numRows = 0;
>> int rows = numRows;
>> foreach (var column in group.Columns.Where(column =>
>> column.Fields.Count > rows))
>> {
>> numRows = column.Fields.Count;
>> }
>

0
Comment actions Permalink

I've checked it on the latest 5.1 build and I got a slightly different result:



var numRows = 0;


foreach(var column in group.Columns.Where(column => column.Fields.Count > numRows))

{

numRows = column.


Fields.Count;

}



which doesn't seem to be incorrect for me. Yes, there is a code issue which is marked by ReSharper but this is a false alarm. Unfortunately, ReSharper currently cannot detect when accessing modified closure is not an issue.
However, ReSharper could be more clever and detect that Max method could be generated to replace the foreach. We'll probably fix it.

0
Comment actions Permalink

Thanks for the info.  Yes, that's indeed a better result.

I should point out that the error we saw was using ReSharper 5.0 (release
version) ... this developer had to back off of 5.1 because he saw
increasingly bad performance in his asp.net projects to the point that he
couldn't work effectively, and reverting back to 5.0 fixed the issue for him
(I posted about this elsewhere in one of the performance threads I think).

Thanks for your follow up!


"Valentin Kipiatkov" <valentin@intellij.com> wrote in message
news:21182911.105521285154099754.JavaMail.devnet@domU-12-31-39-18-36-57.compute-1.internal...

I've checked it on the latest 5.1 build and I got a slightly different
result:

>
>

var numRows = 0;

>
>

foreach(var column in group.Columns.Where(column => column.Fields.Count
> numRows)){
numRows = column.

>
>

Fields.Count;}

>
>
>
>

which doesn't seem to be incorrect for me. Yes, there is a code issue
which is marked by ReSharper but this is a false alarm. Unfortunately,
ReSharper currently cannot detect when accessing modified closure is not
an issue.
However, ReSharper could be more clever and detect that Max method could
be generated to replace the foreach. We'll probably fix it.

>

---
Original message URL: http://devnet.jetbrains.net/message/5273042#5273042


0

Please sign in to leave a comment.