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

Implement named argument for functions #14575

Merged
merged 11 commits into from
Feb 13, 2023
Merged

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Oct 17, 2022

On top of #14475

Not implemented:

  • Named arguments for templates
  • Allowing default parameters in the middle, with trailing non-default parameters
  • Proper overload resolution

The last one is tricky because of how it currently works:

void foo(int x, int* y);
void foo(int* y, int x);

foo(x: 0, y: null); // should be ambiguous

The problem is that FuncDeclaration.leastAsSpecialized compiler simply creates a dummy argument list to see what happens when one overload gets called with the types of the other. This doesn't take into account the re-ordering based on named parameters, so it will think whatever overload is tried first is better.

@dkorpel dkorpel added Review:WIP Work In Progress - not ready for review or pulling Severity:New Language Feature labels Oct 17, 2022
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @dkorpel! 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 + dmd#14575"

@maxhaton
Copy link
Member

The last one is in fact so tricky that I think we should refactor the overload resolution code first. The compiler is a complete cowboy logically when it comes to resolving this stuff.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Nov 3, 2022

I just stumbled upon this issue: https://issues.dlang.org/show_bug.cgi?id=15692 . It asks for in-place struct initialization so if I'm not mistaken, implementing named parameters is going to fix this issue also, maybe you can link it.

@dkorpel
Copy link
Contributor Author

dkorpel commented Nov 3, 2022

I just stumbled upon this issue: https://issues.dlang.org/show_bug.cgi?id=15692 . It asks for in-place struct initialization so if I'm not mistaken, implementing named parameters is going to fix this issue also, maybe you can link it.

This PR doesn't implement that yet, but I might actually start doing that first since I can re-use logic from StructInitializer and don't have to worry about partial ordering of overloads.

@ntrel
Copy link
Contributor

ntrel commented Jan 6, 2023

it will think whatever overload is tried first is better.

For now how about just error in that scenario when named arguments are supplied? It's still useful even if it can't be used to choose an overload.

@maxhaton
Copy link
Member

maxhaton commented Jan 6, 2023

it will think whatever overload is tried first is better.

For now how about just error in that scenario when named arguments are supplied? It's still useful even if it can't be used to choose an overload.

I mostly agree with that.

I also think we should get all of this in-tree. I still think that parsing "foo: 3" as an expression is really weird (and basically guaranteed require a change of course when considering any non-trivial error messages or execution in the right order - it's argument-lexical) but better to have something to change than start again.

@dkorpel
Copy link
Contributor Author

dkorpel commented Jan 6, 2023

I still think that parsing "foo: 3" as an expression is really weird (and basically guaranteed require a change of course when considering any non-trivial error messages or execution in the right order - it's argument-lexical)

I still don't know what error messages you're talking about, but in #14776 I switched to a parallel array of Identifier similar to StructInitializer. That should also make it easy to pass it to leastAsSpecialized to fix overload resolution with named arguments.

@@ -1398,7 +1398,10 @@ private Expression resolvePropertiesX(Scope* sc, Expression e1, Expression e2 =

if (!e1.type)
{
error(loc, "cannot resolve type for %s", e1.toChars());
if (e1.isNamedArgExp())
error(loc, "named argument `%s` not allowed here", e1.toChars());
Copy link
Member

Choose a reason for hiding this comment

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

Could one ever get here? assert?

@maxhaton
Copy link
Member

maxhaton commented Jan 6, 2023

I still think that parsing "foo: 3" as an expression is really weird (and basically guaranteed require a change of course when considering any non-trivial error messages or execution in the right order - it's argument-lexical)

I still don't know what error messages you're talking about, but in #14776 I switched to a parallel array of Identifier similar to StructInitializer. That should also make it easy to pass it to leastAsSpecialized to fix overload resolution with named arguments.

Any error message that happens after semantic analysis in dmd can end up looking weird because the compiler doesn't keep track of the original expression (it would be nicer if it freed it). In original scheme presented (IIRC), a CallExp using named arguments would be resolved to an Array of expressions with no additional context, thus no additional context could be given in an error message if something fails either during the semantic analysis of that call or even an entirely unrelated expression containing a CallExp (user then sees their function called with args they don't recognize).

I introduced a PR to rectify this slightly a few weeks ago (Aimed at making of an array literal resolved from a manifest constant print the identifier of that manifest constant rather than a the whole array - which is useless).

A parallel set of symbols (class NamedArg : ASTNode) is how I did it, I think this should too.

Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Looks good at a first glance, however, @WalterBright should probably take a look at it.

Another approach is to simply merge this behind a preview switch, announce it on the forums, fix any reported bugs and make the preview the default after a few releases.

@dkorpel
Copy link
Contributor Author

dkorpel commented Jan 31, 2023

I prefer this to be merged soon so I can continue working on it. If it's not ready when master gets cut for 2.103.0, I'll put it behind a preview switch on the stable branch.

@dkorpel dkorpel force-pushed the named-arg-resolve branch 2 times, most recently from 658d470 to 6442ad2 Compare February 10, 2023 14:55
Copy link
Member

@WalterBright WalterBright left a comment

Choose a reason for hiding this comment

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

along with the small nits I mentioned

@dlang-bot dlang-bot merged commit 624710c into dlang:master Feb 13, 2023
@dkorpel dkorpel deleted the named-arg-resolve branch February 13, 2023 13:37
@ibuclaw
Copy link
Member

ibuclaw commented Mar 3, 2023

This PR caused a regression https://issues.dlang.org/show_bug.cgi?id=23758

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants