Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std.math: hide parameter names for common math functions #7480

Closed
wants to merge 1 commit into from

Conversation

marler8997
Copy link
Contributor

@marler8997 marler8997 commented May 13, 2020

In anticipation of named parameters, this is an idea to establish a pattern to keep parameter names out of the API when it makes sense. Some math functions like floor and abs that only take one parameter are a good example of functions where the name of the parameter is unhelpful to include in the function's "interface". This PR can serve as a place to debate the pros and cons of such a pattern, and a jumping off point for updating druntime/phobos with this pattern if it is accepted by the community.

@marler8997 marler8997 requested a review from ibuclaw as a code owner May 13, 2020 19:53
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7480"

@benjones
Copy link
Contributor

This seems unnecessary to me. We're trying to prevent users from having their code broken if they use named arguments for a 1 arg math function... Both parts seem unlikely: who's going to use named args to call sin? Is it really worth uglifying phobos to avoid a possible, future, easily fixed compile time error?

@marler8997
Copy link
Contributor Author

This seems unnecessary to me.

Agreed. Necessary is a very high bar. Named arguments themselves aren't necessary, in fact the D language itself isn't necessary. Instead I think we should be asking whether it's an improvement.

We're trying to prevent users from having their code broken if they use named arguments for a 1 arg math function... Both parts seem unlikely:

Yes it's unlikely. But the purpose of this PR isn't to fix these 2 functions in particular, but to establish a pattern for all of druntime/phobos whenever we come across functions whose parameter names shouldn't be apart of the API.

Is it really worth uglifying phobos to avoid a possible, future, easily fixed compile time error?

How important are clean interfaces to you? From personal experience, clean minimal interfaces are one of the most important aspects of programming when it comes to libraries and large projects.

In anticipation of named parameters, this is an idea to establish a pattern to keep parameters out of the API when it makes sense.  Math functions that only take one parameter are a good example of functions where the name of the parameter is unhelpful to include in the function's "interface".
@benjones
Copy link
Contributor

Yes it's unlikely. But the purpose of this PR isn't to fix these 2 functions in particular, but to establish a pattern for all of druntime/phobos whenever we come across functions whose parameter names shouldn't be apart of the API.

Sure, and I think this pattern would make phobos less readable/maintainable. It reminds me of a C++ STL impl that has to throw underscores in front of everything. It seems like an overreaction to me that's solving a problem that is unlikely to exist

How important are clean interfaces to you? From personal experience, clean minimal interfaces are one of the most important aspects of programming when it comes to libraries and large projects.

By minimal, do you mean reducing character count by omitting the parameter name? I'm not distracted by a 1 argument math function using the name x for a parameter name. I've seen functions of x since algebra class.

I get what you're trying to present here, and appreciate it, but for me this example shows that "parameter names as part of the API" is generally not a problem. I suspect a function that takes >3 arguments would be possibly more compelling

@wilzbach
Copy link
Member

Hmm not sure about this one, but I think at the very least we would need to touch ddoc to display something nicer than _param_0 as this would be very confusing for newcomers.

@marler8997
Copy link
Contributor Author

marler8997 commented May 13, 2020

By minimal, do you mean reducing character count by omitting the parameter name? I'm not distracted by a 1 argument math function using the name x for a parameter name. I've seen functions of x since algebra class.

Oh no, I mean "minimal interface". Imagine adding extra parameters to a function that don't do anything, that would not be a minimal interface.

I get what you're trying to present here, and appreciate it, but for me this example shows that "parameter names as part of the API" is generally not a problem. I suspect a function that takes >3 arguments would be possibly more compelling

Ok I think we are mostly in agreement, we just have slightly different priorities. For me, a libarary's interface/API (the information shared between the application and the library) is one of the most important parts of a library. To me, having to type _param_0 or add an alias declaration inside a function is a small prices to pay when weighed against having a simpler function interface.

@marler8997
Copy link
Contributor Author

Hmm not sure about this one, but I think at the very least we would need to touch ddoc to display something nicer than _param_0 as this would be very confusing for newcomers.

Still doesn't seem to work even with _param_0. Along with a nice way to handle ddoc, it sounds like we would also need to make _param_X part of the language spec rather than just an implementation detail.

@benjones
Copy link
Contributor

Ok I think we are mostly in agreement, we just have slightly different priorities. For me, a libarary's interface/API (the information shared between the application and the library) is one of the most important parts of a library. To me, having to type _param_0 or add an alias declaration inside a function is a small prices to pay when weighed against having a simpler function interface.

But, for 99.99% of users the interface is still sin(anAngleICareAbout), so this only affects the documentation for those users (making it slightly worse, IMO), and prevents a user from writing sin(x:anAngleICareAbout), which is unlikely to happen (and if they want to do it, risking a future API break). A user still has to pass a number, so I don't understand how your point about minimizing the interface applies here.

@marler8997
Copy link
Contributor Author

But, for 99.99% of users the interface is still sin(anAngleICareAbout), so this only affects the documentation for those users (making it slightly worse, IMO),

We don't currently have a solution for the doc so it's currently not worse or better :)

and prevents a user from writing sin(x:anAngleICareAbout), which is unlikely to happen (and if they want to do it, risking a future API break).

Right, we removed that possibility from the function's interface.

A user still has to pass a number, so I don't understand how your point about minimizing the interface applies here.

You just showed the example that would no longer be supported. That's minimizing the function's interface.

Also sin is not a good example as I think that one should have a name. Since it accepts radians rather than degrees, naming the parameter by the unit would be helpful.

@@ -566,7 +566,7 @@ enum real SQRT1_2 = SQRT2/2; /** $(SQRT)$(HALF)
*
* Params:
* Num = (template parameter) type of number
* x = real number value
* _param_0 = real number value
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work, and could also be confusing. We would need to figure out how to support documenting nameless parameters in ddoc.

if ((is(Unqual!Num == short) || is(Unqual!Num == byte)) ||
(is(typeof(Num.init >= 0)) && is(typeof(-Num.init))))
{
alias x = _param_0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like _param_0 is an implementation detail rather than part of the spec. We would need to make this part of the spec before establishing a pattern that uses it I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many things that are unnamed in D get a name. That's a huge leak of abstraction, and shouldn't happen, but does due to the compiler having lacking abstractions. I had a go at solving some of the problems it introduces a bit ago but hell broke loose. But if anything, I believe we want to move away from exposing compiler-generated names, instead of relying on it.

If one want to access unnamed parameters, it should be via a traits / is expression.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And regarding documentation, since an identifier cannot start with a digit, we can just use the index. It would need a fix to DDOC, that's all.

@Geod24
Copy link
Member

Geod24 commented May 15, 2020

Why would one even use named parameter for single-argument function ? Why would one do it for unambiguous types or to unambiguous functions ? Named parameters are great for basic types (ignore_error: true or distance_in_kms: 30). It doesn't mean they should be used everywhere.

@atilaneves
Copy link
Contributor

I don't think this change makes much sense. Even if the named arguments DIP gets approved (which is by no means guaranteed), single parameter functions aren't going to be used that way unless they're badly named. If we introduce named parameters and a way to make some of them positional-only like Python, then sure.

@marler8997
Copy link
Contributor Author

single parameter functions aren't going to be used that way unless they're badly named

I'm not sure I agree. For example, I have another PR open (#7481) which changes the name of the single-parameter trigonometry functions from x to radians.

sin(x:90);
// vs
sin(radians:90);

In that example if you understand radians you'll see they probably meant PI/2 instead of 90.

Any function that has a unit-specific parameter could find it useful, another example would be the C sleep function:

sleep(3);
// VS
sleep(seconds:3);

The other example are boolean flags, i.e.

do_the_thing(true);
// vs
do_the_thing(enableLogging:true);

When a function has alot of parameters, named parameters can help distinguish what each argument means, but there are other use cases that apply to functions with single parameters as well.

@n8sh
Copy link
Member

n8sh commented May 16, 2020

Some math functions like floor and abs that only take one parameter are a good example of functions where the name of the parameter is unhelpful to include in the function's "interface".

Calling it _param_0 is substantially worse than calling it x. This PR is bad.

@ibuclaw
Copy link
Member

ibuclaw commented May 16, 2020

If we introduce named parameters and a way to make some of them positional-only like Python, then sure.

Python like is probably the way to go, don't see any benefit making all parameters positional without any explicit annotation.

@atilaneves
Copy link
Contributor

For example, I have another PR open (#7481) which changes the name of the single-parameter trigonometry functions from x to radians.

I don't think that one is useful either.

In that example if you understand radians you'll see they probably meant PI/2 instead of 90.

I don't know what to say to someone expecting to call sin with anything other than radians.

Any function that has a unit-specific parameter could find it useful, another example would be the C sleep function:

Any function that takes a unit like that shouldn't take an integer. At the very least it should be renamed so that the unit is in the function name (e.g. sleepForSeconds). In the case of the C function, which we can't rename, then sure. In the case of such a function existing in Phobos, I'd recommend deprecating it and adding a replacement that does the right thing.

The other example are boolean flags, i.e.

std.typecons.Flag.

When a function has alot of parameters, named parameters can help distinguish what each argument means,

I agree.

but there are other use cases that apply to functions with single parameters as well

If there are, those functions should be rewritten.

@dnadlinger
Copy link
Member

(Responding here, even though this pertains more to #7481.)

I don't know what to say to someone expecting to call sin with anything other than radians.

Explicitly specifying the units can still be useful for clarity in situations where data from multiple sources/devices is mixed. In my day job, I frequently write code that (through no fault of my own) uses radians, degrees as well as turns to specify angular quantities. Why not give people the option to be explicit if they want to? It's not like anyone would want to write sin(x: 1.23) anyway.

@atilaneves
Copy link
Contributor

Explicitly specifying the units can still be useful for clarity

I agree, but radians aren't units.

I frequently write code that (through no fault of my own) uses radians, degrees as well as turns to specify angular quantities. Why not give people the option to be explicit if they want to?

sin(oopsThisIsInDegrees.radians);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants