G-Research / fsharp-analyzers

Analyzers for F#
https://g-research.github.io/fsharp-analyzers/
Apache License 2.0
14 stars 1 forks source link

Failing test case #30

Closed nojaf closed 10 months ago

nojaf commented 10 months ago

I believe I found a problem with the GRA-VIRTUALCALL, If I replace the Seq.collect the code no longer compiles. Could you take a look @dawedawe?

Smaug123 commented 10 months ago

Ah - I may have misunderstood something you were saying earlier. Regardless of the adherence-to-spec of the analyser in this particular case, converting Seq.collect into List.collect or Array.collect is a genuine behaviour change ("eagerly evaluate and allocate") which we probably shouldn't just blindly suggest people do. I intended specifically things like Seq.exists or Seq.fold, which eagerly evaluate the seq and don't have to rematerialise the whole seq in memory anyway, and similarly Set.contains which is inherently much more efficient due to performing a binary search rather than a linear search. For example, I can think of no use case for Seq.fold rather than List.fold when the argument is a list. Seq.collect, by contrast, does have use cases even when applied to lists.

nojaf commented 10 months ago

Yes, we need to go back to the drawing board and be more specific about when the analyzer should report a case.

dawedawe commented 10 months ago

@Smaug123 I went over the functions and tried to come up with a save subset which I hope is okay to warn about:

Here's the set for the Seq functions with equivalents in all collections (Array|List|Set):

Seq.length // ~ Set.count
Seq.max // ~ Set.maxElement
Seq.min // ~ Set.minElement
Seq.contains
Seq.exists
Seq.foldBack
Seq.fold

Here's the set for the Seq functions with equivalents in Array and List modules:

Seq.average
Seq.head
Seq.tryHead
Seq.last
Seq.tryLast
Seq.exactlyOne
Seq.tryExactlyOne
Seq.sum
Seq.averageBy
Seq.compareWith
Seq.countBy
Seq.exists2
Seq.find
Seq.findBack
Seq.findIndex
Seq.findIndexBack
Seq.fold2
Seq.foldBack2
Seq.forall2
Seq.item
Seq.iteri
Seq.iter2
Seq.iteri2
Seq.maxBy
Seq.minBy
Seq.pick
Seq.reduce
Seq.reduceBack
Seq.sumBy
Seq.tryFind
Seq.tryFindBack
Seq.tryFindIndex
Seq.tryFindIndexBack
Seq.tryItem
Seq.tryPick
Smaug123 commented 10 months ago

Yep, those look good.