dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.91k stars 4.63k forks source link

API Proposal around Roslyn Analyser and F# compiler RFC RequireNamedArgumentAttribute #51451

Open smoothdeveloper opened 3 years ago

smoothdeveloper commented 3 years ago

Background and Motivation

An approved F# suggestion is being drafted as an RFC that enforces method call sites to carry argument name when a method is defined with [RequireNamedArgument].

And a similar request was made for C#, but rejected on the ground of "analyser fits the bill".

I'd like the BCL & runtime team to consider if it makes sense to bake the attribute into the BCL rather than going with it in FSharp.Core and letting the non F# users deal with more artisanal approach (anyone coming with their own analyser and attribute couple) while the motives applying to F# in terms of API design apply equally well into the whole dotnet ecosystem (in my opinion).

If there is interest for the attribute to be part of BCL (to cut the concern of what I called artisanal approach above), I'll bring more content to this API proposal based on work so far on the F# RFC and the expected formalism for API proposals.

cc'ing two suspects that you may engage with at Microsoft with a bit of background: @CyrusNajmabadi & @cartermp in case they'd like to engage / nudge in a particular direction.

Proposed API

Name of the attribute subject to discussion, the suggestion stemming from F#, the choice is based on existing such attributes.

namespace System.Runtime.CompilerServices
{
+.   /// Requires argument names to be employed at call site of the member having the attribute defined upon.
+    public class RequireNamedArgumentAttribute : Attribute {
+.       public RequireNamedArgumentAttribute() {}
+.   }
}

Usage Examples

Take a C# code generators that reads .sql file, extracts metadata from the DB server (say in similar fashion as done through sys.sp_describe_undeclared_parameters and sys.sp_describe_first_result_set in context of Microsoft own's SQL Server) and generates bindings of various shapes for calling SQL code in those files, in a type safe manner.

--[ sqlfile initial state ]-------------------------------
declare @theNumerator float
declare @theDenominator float
set @theNumerator = @numerator
set @theDenominator = @denominator
select @theNumerator / @theDenominator

--[ sqlfile after edit ]-------------------------------
declare @theNumerator float
declare @theDenominator float
set @theDenominator = @denominator // swapped this line, did not change anything else
set @theNumerator = @numerator
select @theNumerator / @theDenominator

the generated code would initially have such signature (forgive that the sample is just for purpose of explaining rippling effect of adjusting the parameters in the signature):

public float ExecuteNameOfTheQuery(IDbContext db, float numerator, float denominator) {
/* have fun with ADO.NET */
}

but after the edit, it would be turned into:

public float ExecuteNameOfTheQuery(IDbContext db, float denominator, float numerator) {
/* have fun with ADO.NET */
}

The current results, without an analyser, or compiler warning or even error, is that all call sites have their parameter flipped.

With the suggestion, the methods would be decorated with [RequireArgumentName] and the callsites would need (in order to comply with static analysis or compile time checking) to be always written such as:

ExecuteNameOfTheQuery(db: db, numerator: someValue, denominator: someOtherValueWhichIsNeverZero);

This is one straight forward use case, but looked at more generally, the feature enables APIs that mimmic some of the design choices in languages such as :

to always interleave the argument names with the calls, in those languages, I believe the argument names are also part of the overload resolution, while it doesn't in C#, but this is more about the API feel.

This is also useful for enabling more "business DSL" like looking code on key parts of some of the application code.

This is useful when:

To address @jkotas concern:

the proposal should include guidance on where the library authors (including the dotnet/runtime libraries)

An extreme example would be String.Substring (don't do it!), where the start index and length would be enforced, and suddenly, someone figured that the length should be the second parameter.

I'm sure some of the more "application feely" frameworks, maybe in the web or UI department, may benefit from the feature, in terms of keeping some aspects of the API calls more understandable in any contexts.

Conceptually, all of the Xamarin stack that wraps Objective C APIs could even have this, if there was a mood of making the client dotnet code most identical / one to one translation of using those APIs natively.

I've made a small "language zoo" section in the initial F# RFC:

https://github.com/fsharp/fslang-design/blob/master/drafts/FS-1095-requirenamedargumentattribute.md#language-zoo

Alternative Designs

In F# we've discussed a lot about it, and F# has several more constructs than C# (units of measure, single case DU, records with global inference on set of field names), but they either have runtime cost, or a distortion of usage of those constructs, and don't lean well for the OO feel that this feature intends to give, while remaining a 0 cost abstraction.

In C# the alternative choices would be:

the third approach would look like this at callsite:

ExecuteNameOfTheQuery(dbArg(db), numeratorArg(someValue), denominatorArg(someOtherValueWhichIsNeverZero));

This is unlikely to get traction, and would be used in very little cases.

Risks

For first risk: this is already actual on so many more impactful things (such as record syntax in C# not liking F# records, however those are already existing in much many more codebases than C# records exists) so I don't give it much attention in the context of this API proposal

For the second risk: this is probably a good thing that BCL and compiler teams agree on things that impact the feel of APIs of every dotnet codebases, and has potential to increase the safety, readability, aspects of some of those APIs, down to the actual application code, including libraries at large in the dotnet ecosystem.

dotnet-issue-labeler[bot] commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ghost commented 3 years ago

Tagging subscribers to this area: @tannergooding See info in area-owners.md if you want to be subscribed.

Issue Details
## Background and Motivation An approved F# suggestion is being drafted [as an RFC](https://github.com/fsharp/fslang-design/blob/master/drafts/FS-1095-requirenamedargumentattribute.md) that enforces method call sites to carry argument name when a method is defined with `[RequireArgumentName]`. And a similar request was made for C#, but rejected on the ground of "analyser fits the bill". I'd like the BCL & runtime team to consider if it makes sense to bake the attribute into the BCL rather than going with it in FSharp.Core and letting the non F# users deal with more artisanal approach (anyone coming with their own analyser and attribute couple) while the motives applying to F# in terms of API design apply equally well into the whole dotnet ecosystem (in my opinion). * suggestion for C#: https://github.com/dotnet/csharplang/discussions/1005 rejected for the analyser route * RFC for F#: https://github.com/fsharp/fslang-design/blob/master/drafts/FS-1095-requirenamedargumentattribute.md * discussions so far on the F# RFC https://github.com/fsharp/fslang-design/discussions/538 If there is interest for the attribute to be part of BCL (to cut the concern of what I called artisanal approach above), I'll bring more content to this API proposal based on work so far on the F# RFC and the expected formalism for API proposals. cc'ing two suspects that you may engage with at Microsoft with a bit of background: @CyrusNajmabadi & @cartermp in case they'd like to engage / nudge in a particular direction.
Author: smoothdeveloper
Assignees: -
Labels: `api-suggestion`, `area-System.Runtime`, `code-analyzer`, `untriaged`
Milestone: -
smoothdeveloper commented 3 years ago

@jkotas / @jeffschwMSFT / @CyrusNajmabadi /@cartermp any feedback on this?

Is it something we can get better scoped, pushed for later, binned within next couple of weeks?

Is the BCL fine with the "artisanal approach" of attributes/analysers couples sprouting for this or would rather have a mere canonical attribute defined in the BCL itself.

Thanks!

jkotas commented 3 years ago

Analyzers go through the same review process as APIs: https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md. It would help if you reformat the proposal in this issue to follow the template. In particular, the proposal should include guidance on where the library authors (including the dotnet/runtime libraries) should use this attribute.

smoothdeveloper commented 3 years ago

@jkotas, thanks a lot for the feedback, it makes sense and I'm happy to hear that there is formal process in place for API suggestions review (it is more foggy, but still effective in F# land).

I've updated the main description, please give me feedback if the formalism can be improved before it lands in a design meeting.

Also, if those design meetings are sometimes open to external contributors for the aim of discussing the issue in detail, I'd be happy to join such meeting.

jkotas commented 3 years ago

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/2220 is a very similar proposal to this one. It should be included in alternative designs. Note that it does not require any attributes, it is only driven by heuristics.

jkotas commented 3 years ago

More existing art:

https://www.jetbrains.com/help/rider/Argument_Style.html https://www.jetbrains.com/help/rider/Inline_Parameter_Name_Hints.html#parameter-name-hints

smoothdeveloper commented 3 years ago

Thanks @jkotas, added to the introductory list of references!

I think this highlights it is common concern that some people may want this enforced at callsites of API they design.

FilipToth commented 3 years ago

I like the first approach, Although I do feel like when you need to force named arguments in a method, that’d just bad API design. I guess might be useful in some cases like constructors for classes that need to connect to a server and require credentials etc... I would like to see the exception that gets thrown when the user doesn’t use named arguments (Or whatever we do in that case). About the alternative design, it just doesn't belong to C#.

smoothdeveloper commented 3 years ago

@FilipToth

I would like to see the exception that gets thrown when the user doesn’t use named arguments

In F# proposed implementation, it currently looks like this, could be either warning or error depending how it feels to the BCL and compiler teams:

The method '%s' has the 'RequireArgumentNameAttribute' attribute specified, use the named arguments syntax (e.g. 'MethodName(x = value)').

FilipToth commented 3 years ago

In F# proposed implementation, it currently looks like this, could be either warning or error depending on how it feels to the BCL and compiler teams:

Sounds very reasonable. I don't think an exception needs to be thrown as this is nothing “critical”, just code styling. Thanks for the clarification.

smoothdeveloper commented 3 years ago

Advantage of defining the attribute in the BCL in context of compiler or analyser implementation:

https://github.com/fsharp/fslang-suggestions/issues/414#issuecomment-730700801

For usage with Entity Framework:

https://github.com/fsharp/fslang-suggestions/issues/414#issuecomment-873813200

@jkotas, is this suggestion scoped for review / discussion or are there concerns so far?

jkotas commented 3 years ago

It is up to @dotnet/area-system-runtime to drive the decision on whether this style analyzer and accompanying attribute is worth having in the core platform.

To me, it feels very niche, it would be typically just compensating for bad API design as @FilipToth said, so it is not a clear cut.

tannergooding commented 3 years ago

This issue does come up from time to time but I'm not convinced that requiring the argument name to be specified is the correct approach. A related place this shows up is in various Copy APIs where we haven't always been consistent on whether dest or src comes first (or where users have their own notions).

I wonder how much of this could be covered without an attribute, simply by doing analysis such as:

FilipToth commented 3 years ago

Yes, this API is mostly compensating for bad design, but I think it would be beneficial. Maybe instead of throwing a warning, how about something a little more subtle? Something just enough to say: "Hey, consumers of this API. We recommend using named arguments on the call sites of this method.", but not cause any "warning-pollution" when compiling. Also, one thing to consider is that F# focuses a lot more on the functional side of programming, so in F# this API automatically introduces more value. (Please correct me if I'm wrong) Maybe just show it in Intellisense? Although, that would be a lot harder to implement. I'd love to see the more people @dotnet/area-system-runtime share their opinions, just like @jkotas already said. Then we can hopefully run this through the API review team and see how they feel about this.

smoothdeveloper commented 10 months ago

@tannergooding, I think having an attribute under System.Runtime.CompilerServices is not a huge commitment, and will enable the analyzer that care about argument name on certain methods based on heuristics to also look for that attribute.

What is needed to get this suggestion implemented in BCL?

tannergooding commented 9 months ago

What's the status of the F# feature. We can certainly take this to API review if and when the F# feature is looking to be finalized.

It looks like an experimental branch existed, but it was closed without being merged.