Refactoring for _.Property / _.MethodCall() / _.IndexerAccess[idx] shorthand for accessor functions

[edgarfgp]

Now that #13907 is merged would be good to have code-fix for this

• Design

• Implementation

• From fun x -> x.Foo to _.Foo and maybe the other way around too ?

// From
let a5 : {| Foo : int -> {| X : string |} |} -> string = fun x -> x.Foo(5).X

// To
let a5 : {| Foo : int -> {| X : string |} |} -> string = _.Foo(5).X

// From
let a6 = [1] |> List.map(fun x -> x.ToString())

// To
let a6 = [1] |> List.map _.ToString()

[psfinaki]

You mean something like fixing _.Length() to _.Length?

[edgarfgp]

You mean something like fixing _.Length() to _.Length?

Updated the description clarifying the quick-fix intent. Sorry that it was not really clear

[psfinaki]

Ohh okay. So AFAIU there is no diagnostic for "hey, this is an old way of doing stuff, use the new one" - hence this will need an analyzer first.

[T-Gro]

Code-fixes react to diagnostics, i.e. the code would have to be producing at least a warning or similar in the first place.
An action which chages the "style" without affecting behavior falls into the Refactoring bucket.

[vzarytovskii]

It should be a code action which is a result of code analyzer.

[edgarfgp]

Perfect 👍🏻. Seem a good candidate for the upcoming Analyser sdk

[psfinaki]

Well it could be done similarly to this: #16079

So analyzer + code fix. Refactorings are more invasive code actions :)

[T-Gro]

No, that is not the difference. They can both have various levels of invasiveness.

Codefix needs a diagnostics first a should change wrong code into incorrect one, refactoring does not and can typically exists two times in symmetrical variants.

It is fine if a 3rd party creates their own analyzer, but in my opinion this would be wrong:
It would be telling that existing lambda syntax (fun x -> x.MethodCall().Property.Indexer[42]) needs a warning.

The underscore dot lambda syntax is not meant as a general replacement, the old lambda syntax is still correct and considered good F# code.

Hence: It should be a refactoring, not fixing a diagnostical issue.

[vzarytovskii]

No, that is not the difference. They can both have various levels of invasiveness.

Codefix needs a diagnostics first a should change wrong code into incorrect one, refactoring does not and can typically exists two times in symmetrical variants.

It is fine if a 3rd party creates their own analyzer, but in my opinion this would be wrong:

It would be telling that existing lambda syntax (fun x -> x.MethodCall().Property.Indexer[42]) needs a warning.

The underscore dot lambda syntax is not meant as a general replacement, the old lambda syntax is still correct and considered good F# code.

Hence: It should be a refactoring, not fixing a diagnostical issue.

I think we should introduce another term - code action (as it is in the LSP), which is not necessary a diagnostic driven, not it's necessary a refactoring (as in factoring something in/out). Just an action on a part of AST.

[tboby]

Code actions have a kind: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeActionKind

/**
	 * Empty kind.
	 */
	export const Empty: [CodeActionKind](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeActionKind) = '';

	/**
	 * Base kind for quickfix actions: 'quickfix'.
	 */
	export const QuickFix: [CodeActionKind](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeActionKind) = 'quickfix';

	/**
	 * Base kind for refactoring actions: 'refactor'.
	 */
	export const Refactor: [CodeActionKind](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeActionKind) = 'refactor';

but I'm struggling to find anywhere they are defined properly.

[T-Gro]

I think we should introduce another term - code action (as it is in the LSP), which is not necessary a diagnostic driven, not it's necessary a refactoring (as in factoring something in/out). Just an action on a part of AST.

If the action does not change behavior (the one proposed here does not) and it is a matter of stylistic && subjective improvement (this issue is), it meets the definition of a refactoring. Even more so if it is small (it is here) and can be proven safe.

If we would have any big invasive actions (I do not think we have them though, neither present nor planned), they could fit the term "Redesign" - if we ever need such a thing.

CodeFix - fixes code, has to be wrong (= w/ diagnostics) first
Refactoring - subjectively improves code without changing behavior and without changing outer API
Redesign - can change public API in order to get to subjectively better code

(example of a refactoring is this issue, example of a redesign would be converting tuples to a record)

[psfinaki]

I really don't see how this is different from the recent parentheses analyzer + code fix. let test = 2 + (3 * 4) is also

still correct and considered good F# code

Same with redundant opens, or overqualified types. It's just IDE diagnostics there.

That said, I relate to Vlad's thinking - it's all just code actions with different triggers and the lines are blurring.

[T-Gro]

It's different in not being a generally applicable "fix"..

If someone offered to removed all redundant opens, overqualified types etc. without affecting program behavior, it is easily imaginable to press yes for the entire solution.

Replacing all lambdas with the shorthand should not fall into being a generally applicable improvement.
It's on the level of C# 'var vs types' or 'inline a variable' -> which are refactorings, not fixes.

[vzarytovskii]

Same with redundant opens, or overqualified types.

No, not the same. Verbose function style is not redundant, as unused opens or fq types.

It's a preference in code style, like C#s { return ...; } vs => ..., and as in C# it should be an asynchronous code action/lightbulb triggered by background low priority analyzer.

[T-Gro]

Yes, that is another good example.
And Vlad, what you describe is exactly what a "refactoring means" - this terminology is used in Roslyn, Rider, VS Code. I do not think F# is big enough to deserve it's own naming of basic terms.

https://github.com/dotnet/roslyn/blob/main/src/Features/CSharp/Portable/UseExpressionBody/UseExpressionBodyCodeRefactoringProvider.cs

[psfinaki]

You can ask VS to replace { return ...; } to => for the whole solution :)

I think it really depends on our own perception of this. I thought of this feature as of a general improvement -> as in, I don't know when making this change can worsen the code -> I'd apply it everywhere with the clear head, same as with unused opens, hence code fix for me.

If we actually do not recommend universally (why?) or brand this as a matter of code style, then it's rather in the refactoring direction. However, in either case, if offered to a user - it should not break the code.

[tboby]

I don't know when making this change can worsen the code -> I'd apply it everywhere with the clear head, same as with unused opens, hence code fix for me.

When you have explicitly named function parameters, it would be unexpected for an IDE to tell you to remove them.

|> List.map (fun raw-> raw.Trim())
|> List.map (fun trimmed ->  trimmed.Pad())
...

Looking at C# for an example of inlining (which is almost what this is right?)

Inlining a pointless (assignment only) temporary variable would be a code action of kind "refactor". I would argue this one should be highlighted to the user (Rider does this, yellow lightbulb). LSP says you do that with isPreferred when "it is the most reasonable choice of actions to take", and it should be connected to the default "fix" key-bind.

Inlining a temporary variable that does something is also a code action of kind "refactor". I would argue this one should not be highlighted to the user (Rider does not, it has it under the Refactor submenu). This one shouldn't be blindly applied, and shouldn't be isPreferred, as if the user deliberately did it line by line then they might want to keep it that way.

I can't think of an example of an inlining quick-fix, as that would require a "problem". I don't think a hint level diagnostic suggesting a stylistic change is really the intention of the quick-fix kind :D. Perhaps if having the pointless assignment caused multiple enumeration or something: then it would be a quick-fix to inline it.

[psfinaki]

When you have explicitly named function parameters, it would be unexpected for an IDE to tell you to remove them.

Hmhm no yeah this is fair enough. I probably wouldn't like it, especially if I'd spent hours to think of a variable name prior to that.
I guess it would be hard to come up with an algorithm to understand where the self-identifier is meaningful and where it's not.

[brianrourkeboll]

This suggestion is definitely more of a refactoring than a code fix, since it is very specific and purely stylistic. Maybe we need an analyzer/code fix/refactoring decision tree in the docs... (Or is there one for C# that we could reuse or link?)

@psfinaki

You can ask VS to replace { return ...; } to => for the whole solution :)

I think it really depends on our own perception of this. I thought of this feature as of a general improvement -> as in, I don't know when making this change can worsen the code -> I'd apply it everywhere with the clear head, same as with unused opens, hence code fix for me.

If we actually do not recommend universally (why?) or brand this as a matter of code style, then it's rather in the refactoring direction. However, in either case, if offered to a user - it should not break the code.

In my experience, certain C# refactorings will happily break code or completely change its behavior if applied, e.g.,

…Although I impressed to see that warning about it changing the code meaning—I'm pretty sure it didn't use to do that!

The funny thing is that the suggested refactoring in that case would not actually change the behavior (other than allocating additional Funcs; DateTimeOffset.Now would still only be called once), whereas this one would change the behavior (DateTimeOffset.Now would now be called multiple times), and the refactoring does not warn that it would:

But yes, a diagnostic analyzer and code fix combo would have greater expectations that it would not suggest a cure worse than the disease.

As for weighing a fix's utility in a given local scenario against the potential downsides of applying it in bulk, sight unseen, I think it would probably be best to outline exactly in which scenarios a given diagnostic is deemed worth emitting, or a fix or refactoring worth applying, if possible.

Sometimes there are multiple valid perspectives, in which case it is possible to add options for the diagnostic, as in, e.g., https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0047-ide0048#options.

Even then, you could argue that enabling a developer to apply a fix in bulk is not the same as forcing or even advising it; the developer can choose to act or not. But if leaving that freedom (and responsibility) to the developer is undesirable, as long as we support .editorconfig configuration, any given diagnostic can be suppressed or configured globally or locally in global or local .editorconfig files.

[psfinaki]

In my experience, certain C# refactorings will happily break code
Yeah once I've starting seeing this happening in C# (to the extent of VS turning off some C# components) I got a bit calmer about F# code actions in that regard :D

or completely change its behavior if applied
That's a remarkable example, thanks! Interesting and valid observations.

So I'd be quite conservative about introducing these kinds of warnings for developers in F#, because:

1. "Change of behavior" might mean different things to people - some people would think in terms of logical flows (me, for example) and others would think that even the generated code (hence allocations etc) stays the same
2. If we bring such warnings, people might start trusting this - and given the examples above, it's easy to mislead them here

In short, expectation management. I'd focus on recommending things that are either always harmless or fix some problems. And if this is not the case, it's a bug. There are a looooot of things we can do it the area of editor experience and I'd argue for prioritization of such bulletproof improvements.