Add an analyzer to recommend using named parameters when passing literals as arguments

[DaRosenberg]

We employ a guideline to always use named parameters when the meaning of an argument is not obvious from the argument itself.

For example, the intent is not very clear here, from just looking at the code:

var employees = await FetchEmployeesAsync(true);

If you're in an IDE, you can of course hover over the method call for a few seconds to get a tooltip, or go to the definition of the method, but it adds friction and delay when reading code. And oftentimes, code is read outside of an IDE (for example, when reviewing a PR or perusing code on GitHub).

But it's much clearer if you do:

var employees = await FetchEmployeesAsync(includeContractors: true);

Or:

var includeContractors = true;
var employees = await FetchEmployeesAsync(includeContractors);

For most cases, this could be formalized as "always use named parameters when passing literal values as arguments in a method call".

It would be nice to have an analyzer for this. Let me know if you think this is a good idea or not. I wouldn't mind taking a stab at implementing it myself, if it doesn't already exist and nobody else is already working on it.

[mavasani]

This seems related to https://github.com/dotnet/roslyn/pull/22010, which was attempted by @jcouv. I think the last comment on the PR https://github.com/dotnet/roslyn/pull/22010#issuecomment-355157341 mentions the issue with this request - the analyzer could end up being extremely demanding. So, I think we would need to implement this with following additions:

1. Ability to exclude certain methods: Have an .editorconfig option to ignore validation for specific method or property symbol, with reasonable defaults for methods/properties always excluded from this analyzer. For example, Debug.Assert would be a classic case that users don't care about and do not want named argument (maybe we could ignore all methods with a single boolean parameter by default, but that would be too aggressive I think)
2. Ability to exclude parameter types: Have an editorconfig option to ignore specific parameter types, such as bool, which can come in lot of methods. I am not sure if users would use this option as much as the previous one, so probably this can be implemented later based on feedback.

Tagging @Evangelink in case he is interested in implementing this analyzer. I have always wanted to enable this analyzer for this repo itself :-)

[jcouv]

If I were to do it again today, maybe I would suppress those diagnostics (or just not enable the enforcement) on test projects altogether. Then maybe we don't need additional configuration (excluding certain methods or types).

@mavasani Let me re-open my old PR, then test on Roslyn with above rule (only enforce in product code) to see if that would be workable.

[DaRosenberg]

If the analyzer excluded certain very common cases, that'd be ideal IMO.

Besides any method with "Assert" in its name as already mentioned, also the case ConfigureAwait(false) comes to mind as one that is so common and well-understood that it would make sense to exclude it.

[mavasani]

@vatsalyaagrawal @jinujoseph @sharwell Can we port this issue to Roslyn? This is a code style analyzer proposal.

[mavasani]

@jcouv Would you like to take this up or would prefer someone from the IDE team to take it up? Thanks!

[jcouv]

It looks like my earlier PR needed more work, as Jason felt we should convert the existing refactoring into a fixer (so that we don't have duplicate fixer and refactoring). I'll leave this to the IDE team. The tests from that PR could probably be re-used though.

[mavasani]

Suggestions from @alrz from https://github.com/dotnet/roslyn/pull/45989#discussion_r455133315

My opinion: #43988 is too much noise, even though it's "usually" a good idea to use named args, it really actualy depends on the context where you need to add named args for literals. Hardcoding a list of methods doesn't seem to be a solution neither. But since out _ is actually hiding a piece of info (while saying that this is not being used), named args can be always helpful.

One heuristic for #43988 I think is that only offer when literals are mixed with other kinds of expressions. so Add(1) or ConfigureAwait(false) won't get flagged but M(a, true, b) does.

[DaRosenberg]

Agree that out _ should always be suggested to use named args.

Not quite sure what to think about @alrz suggests though... I guess it would be better than nothing. I just think there would be a lot of missed cases. I would venture to guess that method calls with only one or more literals as args, are a very common occurrence, which would make this analyzer kind of "toothless" from a guideline enforcement standpoint... but if no-one has a better idea, definitely better than nothing.

[CyrusNajmabadi]

Agree that out _ should always be suggested to use named args.

I personally disagree on this. I haven't run into a case ever where i've wanted this, and i'm generally the person on my team asking people to use named params for clarity.

[mavasani]

I don't believe this analyzer is any longer needed given the work recently done by @akhera99 on inline parameter name hints. It is an awesome feature, and kind of makes the named arguments feature itself redundant. I am going to propose that we close this issue as won't fix.

Marking as needs design review to discuss in next design meeting.

[DaRosenberg]

@mavasani That looks awesome indeed.

In fact, at first glance it might seem as if that even negates the whole idea of named parameters as a language construct. But the fact is, a VS editor adornment (while really cool) is not a real substitute and named parameters will of course remain in the language - for good reason.

Before closing this, I'd consider the following:

• What about when viewing code outside the context of an IDE, such as when reviewing a PR in GitHub or Azure DevOps?
• Or viewing code in third-party tools like profilers etc?
• Is this editor feature coming to other C# IDEs as well in the short term (i.e. VS Code and VS for Mac)?

[jinujoseph]

Design Meeting Notes

The proposed resolution is intentionally deferred, we are going to wait and see what's the adoption feedback is from the user-facing feature (inline parameter name hints) to understand if it's showing up in the right places and if that meets people's needs and to what degree, we do understand that there are certain areas where it can't be used and there will be the clear value of having some analysis for specific cases and If we could find certain rules/patterns which wouldn't be too intrusive to the way projects are maintained over time

[Dreamescaper]

I had mixed feelings with Inline parameter name hints feature in Resharper, and now I have same mixed feelings with it in VS. While it is very convenient, it encourages to write code which is unreadable outside of IDE.

Therefore, I would really like to have this analyzer implemented anyway, disregarding Inline parameter hints feature.

[CyrusNajmabadi]

Therefore, I would really like to have this analyzer implemented anyway, disregarding Inline parameter hints feature.

Note: analyzers are an open ecosystem. Roslyn has introduced and adopted some for some very contentious areas where there are deep community splits on how people want to write and what they want enforced. This style does not seem to be in the same category. As such, i'm not pushing for roslyn to provide thsi. it would actually be extremely easy and straightforward to just write your own analyzer here and use taht to ensure your own code matched whatever rules you think are sensible in your domain.

[ci-vamp]

@DaRosenberg may i ask how you implemented this rule? i would like to enforce it in our codebase

[DaRosenberg]

@ms-vamp I did not implement anything related to this (yet).

[elachlan]

Winforms would absolutely use the rule. It's a part of the code style.

[ci-vamp]

@ms-vamp I did not implement anything related to this (yet).

i see. i am not as familiar with the linting/formatting ecosystem on the c# side of things. i have been trying to wrap my head around how it works. it looks like some pieces are available but i dont know how to use them.

here is some info i found on my search that may help:

• roslyn use named arguments code refactor provider
  • can this be turned into a rule?
• an f# implementation but it requires the method to have an annotation above it.

[CyrusNajmabadi]

@ms-vamp I would recommend reading up on DiagnosticAnalyzer and CodeFixProvider. There are also lots of examples in Roslyn and in the wild. I do not think this is necessarily something we would do ourselves in Roslyn as it likely requires just a lot of customizability on the part of the user. For example, some ability to whitelist certain APIs as not needing named args since the code is so self-evident.

However, if an external team or individual wanted to do this, they could go wild with customization here if they so wanted.

[DaRosenberg]

@CyrusNajmabadi just to double-check here since this issue is tagged as "help needed" — are you saying that if we made the effort to contribute an analyzer and fix for this as a PR, it would still not be accepted into Roslyn?

[CyrusNajmabadi]

@DaRosenberg because of hte complexity here, i would advise not implementing this until a design has been put forth (which you are welcome to do) that the team accepts. If such a design can be made that is simple and clean, then that would greatly increase the odds that we would take this in roslyn. If, however, no such design can be agreed upon (perhaps because there would just need to be far too much customizability here than we're comfortable with), then we would likely not take it.

To give an example, one of our most complex features is use var, which basically only has 3 knobs and which still has a ton of requests to make it even more flexible which we are very resistant to do. I could easily see this feature going far beyond that (including needing some way to whitelist APIs) which would make this very complex and far out of bounds with what we're comfortable with wrt complexity and maintenance costs.

But I could def be wrong here and we'd def consider a proposal.

[DaRosenberg]

@CyrusNajmabadi I might give it a think actually. Is there any specific format or place in which a design proposal should be put forth?

[CyrusNajmabadi]

This issue would be the right place. THere's no specific format, but i would be looking for a fairly detailed listing of how users would configure this in editorconfig and hte sorts of cases where a blanket rule would likely be quite poor for. As mentioned above, there are tons of apis like Debug.Assert, ConfigureAwait (and many more) that this feature would likely be a poor fit for. Currently skeptical about a suitably flexible, while simple, solution here. But would def be up to hearing ideas.

[ci-vamp]

@ms-vamp I would recommend reading up on DiagnosticAnalyzer and CodeFixProvider. There are also lots of examples in Roslyn and in the wild. I do not think this is necessarily something we would do ourselves in Roslyn as it likely requires just a lot of customizability on the part of the user. For example, some ability to whitelist certain APIs as not needing named args since the code is so self-evident.

However, if an external team or individual wanted to do this, they could go wild with customization here if they so wanted.

pardon my ignorance but it has been hard to understand all the components of linting/formatting on the c# side of things (coming from eslint/prettier).

in general i would like to make use of existing roslyn code refactor providers (such as the one i linked as well as line wrapping for methods and chained calls). i see that they are used in VS refactoring suggestions but i would like to execute across the entire solution and then enforce it with rules.

is there information about how to integrate these features into an editorconfig that dotnet format can utilize to operate across an entire solution?

[CyrusNajmabadi]

but i would like to execute across the entire solution and then enforce it with rules.

That's what a DiagnosticAnalyzer/CodeFixProvider gets you. :-)

Code refactorings are targeted, user-invoked, code transformations. They have no rules or enforcement associated with them.

In a case like this, we would likely extract out common helper code, then use that code from both the refactoring and the analyzer/fixer pair.

[elachlan]

Winforms is specifically looking for a DiagnosticAnalyzer/CodeFixProvider that detects a null value passed to a parameter and changes it to have a named parameter.

[uecasm]

I would definitely like an editorconfig rule that can encourage people to use named literals or variables for false/true and perhaps also null/default (though these are perhaps more prone to annoyance). I strongly dislike IDE annotations because they disrupt spacing alignment and don't work in external contexts such as github code reviews. I'm less concerned with other literal values; they're usually a lot more self-evident (and in particular I would generally oppose doing it for string literals).

Perhaps to reduce the potential annoyance a little, there could be an opt-out attribute on the target parameter (or method as a whole), to handle the "sufficiently obvious" cases. Though this will only be effective once the attributes disperse into library implementations (or for tools like ReSharper where such attributes can be applied via external configuration).

[MPCoreDeveloper]

Maybe a bit late but I just released one that analyzes and fixes named arguments as a nuget package : https://www.nuget.org/packages/Posseth.NamedArguments.AnalyzerAndFixer you can find the source here: https://github.com/MPCoreDeveloper/Posseth.NamedArguments.AnalyzerAndFixer

[JimWolff]

Just putting my two cents in there: I would prefer an .editorconfig solution where this could be enabled like: boolean_parameters_must_be_named or something

also a bit of discussion here about it: clean-code-tip-avoid-booleans-as-function-activity

I don't like the suggestion of using an enum in general (for only two options, it has it's own issues with default initialization to 0 and such) and you have to do extra boilerplate code then.

[MPCoreDeveloper]

Just putting my two cents in there: I would prefer an .editorconfig solution where this could be enabled like: boolean_parameters_must_be_named or something

also a bit of discussion here about it: clean-code-tip-avoid-booleans-as-function-activity

I don't like the suggestion of using an enum in general (for only two options, it has it's own issues with default initialization to 0 and such) and you have to do extra boilerplate code then.

I just do everything with an exclusion list and options that can enable/ disable or just ad your own namespaces or method names in the latest version.