Roslyn: 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.