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 std.functional.bind #8376

Merged
merged 2 commits into from
Mar 10, 2022
Merged

Add std.functional.bind #8376

merged 2 commits into from
Mar 10, 2022

Conversation

pbackus
Copy link
Contributor

@pbackus pbackus commented Feb 7, 2022

Fixes Issue 22736 - Add destructuring bind for std.typecons.Tuple tuples


This version works for all struct types, not just std.typecons.Tuple. See #8372 for discussion.

@pbackus pbackus requested a review from andralex as a code owner February 7, 2022 23:18
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @pbackus! 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

Auto-close Bugzilla Severity Description
22736 enhancement Add destructuring bind for std.typecons.Tuple tuples

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

@CyberShadow
Copy link
Member

Thanks!

See #8372 for discussion.

To summarize a lot of it, I'm hesitant about such an addition to Phobos because I can't see many use cases outside tuples, and it is so broad that it allows doing e.g.:

struct Coordinate { float longitude, latitude; }
Coordinate c;
...
c.bind!((latitude, longitude) => /* OOPS */);

Note that the JavaScript equivalent behaves correctly:

let c = {longitude: 12, latitude: 34};
(({latitude, longitude}) => /* OK */)(c);

I think we actually can make the D construct behave like the JavaScript one if we wanted to, which is why maybe that should be a separate discussion from #8372.

@pbackus
Copy link
Contributor Author

pbackus commented Feb 8, 2022

Here's a non-std.typecons.tuple use-case that I almost included in the examples:

int[string] cityPopulations = [
    "New York" : 8_804_190,
    "Los Angeles" : 3_898_747,
    "Chicago" : 2_746_388,
    "Philadelphia" : 1_603_797,
    "Seattle" : 737_015,
    "Boston" : 675_647
];

// Print names of cities with populations > 1mil and < 5mil
cityPopulations.byKeyValue
    .filter!(bind!((name, pop) => pop > 1_000_000 && pop < 5_000_000))
    .map!(bind!((name, pop) => name))
    .each!writeln;
  • byKeyValue uses a non-std.typecons.Tuple struct as its element type, so a Tuple-only version wouldn't work here.
  • Using bind lets us have meaningful names (name, pop) instead of generic ones (key, value).

@CyberShadow
Copy link
Member

byKeyValue uses a non-std.typecons.Tuple struct as its element type, so a Tuple-only version wouldn't work here.

There is std.array.byPair which does return a range of Tuple.

@pbackus
Copy link
Contributor Author

pbackus commented Feb 8, 2022

I think we actually can make the D construct behave like the JavaScript one if we wanted to

Sadly I don't think this is possible in library code without resorting to string mixins, but I'd be delighted to be proven wrong about that.

{
assert(toCtString!0 == "0");
assert(toCtString!123456 == "123456");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not static asserts instead?

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 is copy+pasted from std.sumtype, and it uses runtime assert there too.

Copy link
Member

Choose a reason for hiding this comment

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

Can't this be in a shared module? I don't see the reason for a runtime assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once this is merged I plan to submit a separate refactoring PR addressing these issues. Since this PR introduces a new feature, I want to keep refactoring out of it.

@atilaneves atilaneves added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Mar 7, 2022
@CyberShadow
Copy link
Member

@atilaneves Thanks! Though, I'd love a rationale of your choice.

@atilaneves
Copy link
Contributor

@atilaneves Thanks! Though, I'd love a rationale of your choice.

It's more general, and I didn't see why constrain it to tuples.

Copy link
Collaborator

@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.

This still needs a changelog entry.

@CyberShadow
Copy link
Member

CyberShadow commented Mar 7, 2022

It's more general, and I didn't see why constrain it to tuples.

It's discussed in the other pull request and #8376 (comment). (Not sure if you didn't see the discussion or if you found it unconvincing.)

Edit: Well, I won't say no to a more generic version if the BDFL says the concerns are not worth worrying about 😄 Thanks @pbackus for the nice implementation!

@atilaneves
Copy link
Contributor

It's more general, and I didn't see why constrain it to tuples.

It's discussed in the other pull request and #8376 (comment). (Not sure if you didn't see the discussion or if you found it inconclusive.)

I saw and read it, yes, I was more convinced by the other arguments. I hope that makes sense!

}

/**
* Passes the fields of a struct as arguments to a function.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it confusing that only struct fields are mentioned here. tuples should be mentioned too

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean language tuples (like in AliasSeq) or std.typecons tuples? The latter are just structs.

For language tuples you can just use the Identity template.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was referring to std.typecons.tuples.

import core.lifetime : move;

// Forwards the i'th member of `args`
// Needed because core.lifetime.forward doesn't work on struct members
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can fix core.lifetime.forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not possible, unfortunately—forward takes its argument as an alias template parameter, and so does not have access to the this pointer at runtime.

pbackus added 2 commits March 7, 2022 15:40
Fixes Issue 22736 - Add destructuring bind for std.typecons.Tuple tuples
@pbackus
Copy link
Contributor Author

pbackus commented Mar 7, 2022

@RazvanN7 Added a changelog entry.

@RazvanN7 RazvanN7 merged commit 16cb085 into dlang:master Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Review:Atila Neves Severity:Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants