Add named arguments to struct literals #14776

dkorpel · 2023-01-02T22:41:43Z

Best reviewed per commit, because the big copy-paste Move out resolveStructLiteralNamedArgs does not give a pleasant diff.

Add named argument parsing. This time with a separate Array!Identifier instead of a NamedArgExp, which was disliked in Add named argument parsing support #14475
Make initsem of StructInitializer re-usable
Use the logic in expsemantic of a CallExp on a constructor-less struct

This PR does not do function calls / constructors, that's #14575, which is stalled because checking which of two overloads is more specialized still needs to be adjusted for named parameters.

I haven't started on named template arguments.

dlang-bot · 2023-01-02T22:41:46Z

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#14776"

dkorpel · 2023-01-02T23:25:31Z

Currently fails with tuples:

void main() {
    auto t = tuple("H", "I");
}

Tuple!T tuple(T...)(T values)
{
    return Tuple!T(values); // < (param1, param2) gets rewritten to (null, paramX)
}

struct Tuple(T...)
{
    T values;
    alias values this;
}

Will look at it tomorrow

dkorpel · 2023-01-14T12:01:38Z

@RazvanN7 This is working now. Named arguments for functions are ignored as of this PR, the next step would be to rebase #14575 on top of this to fix that. That should be doable before the 2.103 release, otherwise I could add it behind a preview switch until it's all in a clean state.

compiler/src/dmd/expression.d

RazvanN7 · 2023-01-16T10:11:58Z

compiler/src/dmd/expression.d

+            foreach (i; 0..exps.length)
+                (*names)[i] = null;
+        }
+        if (exps.length != names.length)


Why keep the dead code?

It catches bugs

Note that it's basically an assert, but using if and printf to give a more fluent assert message.

compiler/src/dmd/expression.d

compiler/src/dmd/initsem.d

compiler/test/compilable/named_argumens_struct.d

compiler/test/fail_compilation/fail155.d

compiler/test/fail_compilation/fail158.d

Co-authored-by: Razvan Nitu <razvan.nitu1305@gmail.com>

dkorpel · 2023-01-17T09:54:21Z

@RazvanN7 Anything else?

RazvanN7 · 2023-01-17T11:19:23Z

compiler/src/dmd/expression.d

+            // (UFCS/operator overloading) are fixed to take named arguments into account
+            names.setDim(exps.length);
+            foreach (i; 0..exps.length)
+                (*names)[i] = null;


What are the situations where names.length != exps.length? I assume from your comment that this is here to catch bugs in other places, however, why do you nullify the names array? Also, you're printing the elements in the next branch so you're putting yourself at risk of a segfault.

The next branch won't be taken, because this branch does names.setDim(exps.length);. It nullifies them because setDim doesn't initialize new elements, and you don't want it to search a garbage identifier. I could just nullify the new elements, but it doesn't matter, the names aren't correct anyway. This is not a problem, because named arguments don't exist yet, so there's no code relying on it being correct. This whole branch is just scaffolding to make it not crash for various constructed CallExps.

I don't know all situations where names.length != exps.length. One of them was where a.fun(name: b) gets rewritten to fun(a, b) without also prepending a null name, but I fixed that. There are a lot more, but I'll defer them to the PR that implements named function arguments.

Ok, makes sense, thanks for the explanation.

RazvanN7 · 2023-01-17T11:22:35Z

compiler/src/dmd/expression.d

+        {
+            if (length == 0)
+            {
+                names.remove(index);


This behavior should be documented in the spec PR. I agree that this is the most sane approach, but others might expect that the named arg is attached to the following argument after the empty AliasSeq (and error if the arg already has a name).

RazvanN7 · 2023-01-17T11:30:55Z

compiler/src/dmd/expression.d

+            }
+            foreach (i; 1 .. length)
+            {
+                names.insert(index + i, cast(Identifier) null);


Suggested change

names.insert(index + i, cast(Identifier) null);

names.insert(index + i, null);

That gives an ambiguity error, because insert also has an overload with an Array*

RazvanN7

I guess you will post a documentation PR when named arguments are implemented. Just do not forget to mention that struct literals obey the same rules as any other function with named args.

dkorpel · 2023-01-17T12:12:11Z

I created a project to give an overview, and to help me remember:
https://github.com/orgs/dlang/projects/19/views/1

dkorpel added the Severity:New Language Feature label Jan 2, 2023

dkorpel force-pushed the named-arg-struct branch from 6744923 to 528d990 Compare January 2, 2023 23:22

dlang-bot added the Review:Needs Work label Jan 3, 2023

dkorpel mentioned this pull request Jan 6, 2023

Implement named argument for functions #14575

Merged

dlang-bot added Review:Needs Work and removed Review:Needs Work labels Jan 6, 2023

dkorpel force-pushed the named-arg-struct branch from b633451 to ed2c265 Compare January 13, 2023 11:53

dlang-bot removed the Review:Needs Work label Jan 13, 2023

dkorpel force-pushed the named-arg-struct branch 5 times, most recently from ddec9a4 to b5650db Compare January 13, 2023 19:15

dkorpel marked this pull request as ready for review January 14, 2023 11:47

dkorpel requested a review from ibuclaw as a code owner January 14, 2023 11:47

dkorpel mentioned this pull request Jan 14, 2023

Add named argument parsing support #14475

Closed

RazvanN7 reviewed Jan 16, 2023

View reviewed changes

dkorpel added 10 commits January 16, 2023 12:56

Add named argument parsing

75c80ea

Refactor StructInitializer semantic to make it reusable

7a85f87

Move out resolveStructLiteralNamedArgs

792a1cf

Resolve named args in struct literal

44473c6

Add tests

042727e

Check for errors returned by resolveStructLiteralNamedArgs

494174b

Expand names along with tuples

f3df9ee

Update error messages in tests

904d769

Convert tabs to spaces in named arg test

9bff5d0

Fix style of resolveStructLiteralNamedArgs

b984ff3

Clarify 'fixme' comment in expandTuples

41d7a01

dkorpel force-pushed the named-arg-struct branch from b5650db to 88fc33b Compare January 16, 2023 12:12

dkorpel and others added 3 commits January 16, 2023 13:12

Add example to expandTuples documentation

88fc33b

Update compiler/src/dmd/initsem.d

4ad4d34

Co-authored-by: Razvan Nitu <razvan.nitu1305@gmail.com>

Add overlapping initialization supplemental error to Struct Initializer

47d2ea5

dkorpel force-pushed the named-arg-struct branch from 6d34239 to 95a3167 Compare January 16, 2023 12:48

Improve "too many initializers" error message

95a3167

RazvanN7 reviewed Jan 17, 2023

View reviewed changes

RazvanN7 approved these changes Jan 17, 2023

View reviewed changes

dkorpel merged commit 42609ae into dlang:master Jan 17, 2023

dkorpel deleted the named-arg-struct branch January 17, 2023 12:08

CyberShadow mentioned this pull request Jan 18, 2023

dmd.expression: Fix another stray paren in DDoc #14835

Merged

dkorpel mentioned this pull request Feb 16, 2023

Fix inconsistent names and arguments arrays #14889

Merged

This was referenced Mar 23, 2023

Changelog entry for named arguments #15030

Closed

Add grammar for named function arguments dlang/dlang.org#3565

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add named arguments to struct literals #14776

Add named arguments to struct literals #14776

dkorpel commented Jan 2, 2023

dlang-bot commented Jan 2, 2023

dkorpel commented Jan 2, 2023

dkorpel commented Jan 14, 2023

RazvanN7 Jan 16, 2023

dkorpel Jan 16, 2023

dkorpel Jan 16, 2023

dkorpel commented Jan 17, 2023

RazvanN7 Jan 17, 2023

dkorpel Jan 17, 2023

RazvanN7 Jan 17, 2023

RazvanN7 Jan 17, 2023

dkorpel Jan 17, 2023

RazvanN7 Jan 17, 2023

dkorpel Jan 17, 2023

RazvanN7 left a comment •

edited

Loading

dkorpel commented Jan 17, 2023

	names.insert(index + i, cast(Identifier) null);
	names.insert(index + i, null);

Add named arguments to struct literals #14776

Add named arguments to struct literals #14776

Conversation

dkorpel commented Jan 2, 2023

dlang-bot commented Jan 2, 2023

Bugzilla references

Testing this PR locally

dkorpel commented Jan 2, 2023

dkorpel commented Jan 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkorpel commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RazvanN7 left a comment • edited Loading

Choose a reason for hiding this comment

dkorpel commented Jan 17, 2023

RazvanN7 left a comment •

edited

Loading