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

Add named argument parsing support #14475

Closed
wants to merge 1 commit into from

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Sep 23, 2022

Start implementing https://www.dlang.org/dips/1030

Does not do the hard part yet, but it's a start. Named (template) arguments are parsed to a NamedArgExp, which result in a "not implemented" error during semantic analysis.

@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#14475"

compiler/src/dmd/parse.d Outdated Show resolved Hide resolved
compiler/src/dmd/parse.d Outdated Show resolved Hide resolved
@dkorpel dkorpel marked this pull request as ready for review September 23, 2022 10:35
@maxhaton
Copy link
Member

Are the argument pairs really expressions?

*
* Examples: `f(x: 3)`, `g!(T: int)`
*/
extern (C++) class NamedArgExp : Expression
Copy link
Member

Choose a reason for hiding this comment

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

It might make the semantic analysis easier if (I just though of this now) if a NamedArgExp is actually a call expression with named arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Currently this being part of the expression hierarchy sounds like a bad idea

Copy link
Member

Choose a reason for hiding this comment

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

The argument matching needs to be done lexically and semantically so having them as expressions has some potential for wacky shit to happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what you mean with "argument matching needs to be done lexically and semantically".

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 might make the semantic analysis easier if (I just though of this now) if a NamedArgExp is actually a call expression with named arguments.

A CallExp has an Expressions* arguments, where would I store the names of the named arguments if not in a NamedArgExp?

Copy link
Member

Choose a reason for hiding this comment

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

Implementing the semantic analysis for function calls to normal functions is fairly easy, implementing the semantic analysis to anything with an overload (ESPECIALLY a template overload) is absolutely hell on earth because it tries to save references to the arguments all over the place so if you start overwriting them piecemeal as you go through them stuff just randomly breaks.

Copy link
Member

@maxhaton maxhaton Sep 24, 2022

Choose a reason for hiding this comment

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

And think about it, you overwrite the array, any error message, AST dump etc., now prints something which is complete gibberish since it's printing what the compiler lowered the arguments into not how the user wrote them down - even worse, this is actually still wrong because the arguments are supposed to be evaluated in lexical order which is now not the order in the AST.

Copy link

Choose a reason for hiding this comment

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

NamedArgExp as proposed here is nice, for example this can be reused for struct literals or I dont know what else in the future (theoritically).

Copy link
Member

Choose a reason for hiding this comment

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

Still not an expression

Copy link

Choose a reason for hiding this comment

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

Indeed, you have to follow the DIP.

@Geod24
Copy link
Member

Geod24 commented Sep 24, 2022

@dkorpel : My approach, when I looked at this, was to use a struct with two members. Hence I'm a bit reticent of doing a piecemeal implementation unless we have some guarantee this is the path forward.

@dkorpel
Copy link
Contributor Author

dkorpel commented Sep 24, 2022

If it turns out we need a struct instead of a class, we can always change it later.

@maxhaton
Copy link
Member

It should be a struct containing an array of pairs of names and expressions.

It cannot be just an array because there's needs to be an opaque migration path away from the rest of the compiler using .args directly

@maxhaton
Copy link
Member

I think my branch might be on my GitHub somewhere, I've bumped into all of these problems already. We should start by splitting of the affected logic into callsem.d or something first

Start implementing https://www.dlang.org/dips/1030

Named (template) arguments are parsed to a `NamedArgExp`, which result
in a "not implemented" error during semantic analysis.
@dkorpel
Copy link
Contributor Author

dkorpel commented Oct 18, 2022

My approach, when I looked at this, was to use a struct with two members. Hence I'm a bit reticent of doing a piecemeal implementation unless we have some guarantee this is the path forward.

@Geod24 @maxhaton I now have #14575 so you can see where this PR is going towards. Can you re-review based on that?

@thewilsonator thewilsonator added Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Review:Needs Changelog A changelog entry needs to be added to /changelog labels Oct 19, 2022
@RazvanN7
Copy link
Contributor

RazvanN7 commented Nov 1, 2022

@dkorpel I think you should just keep the DIP implementation in a single PR but with multiple, independent commits, if possible. Having the implementation scattered over multiple dependent PRs makes it hard to navigate through the changes.

@RazvanN7
Copy link
Contributor

@dkorpel Are you still working on this?

@dkorpel
Copy link
Contributor Author

dkorpel commented Jan 14, 2023

Parsing is now implemented without NamedArgExp in: #14776

@dkorpel dkorpel closed this Jan 14, 2023
@dkorpel dkorpel deleted the named-arg-parse branch January 20, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants