Skip to content

Conversation

@JinShil
Copy link
Contributor

@JinShil JinShil commented Aug 31, 2019

A "gentler" alternative to #10179

in as a parameter storage class is defined as scope const. However in has not yet
been properly implemented so its current implementation is equivalent to const. Properly
implementing in now will likely break code, so it is recommended to avoid using in, and
explicitly use const or scope const instead, until in is properly implemented.

The use of in as a parameter storage class is already discouraged in the documentation. See https://dlang.org/spec/function.html#parameters

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @JinShil! 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 fetch digger
dub run digger -- build "master + dmd#10382"

@rainers
Copy link
Member

rainers commented Aug 31, 2019

Should the message also be emitted for disabled code? If not, the check needs to be done during semantic analysis.

@12345swordy
Copy link
Contributor

I still don't understand why we can't simply redefine "in" as "const scope" when -dip1000 flag has been applied. Deprecating "in" seems to me a bad decision giving that "inout" and "out" still exist.

@JinShil
Copy link
Contributor Author

JinShil commented Aug 31, 2019

Please understand that we're not deprecating in. All we're doing is telling users not to use it yet, because if and when it is implemented it could break their code. Also, there are a few other ideas floating around to make in more than just const scope (i.e. const scope ref). It's just best if users don't use in until a decision can be made about it.

Right now we're sabotaging users by not informing them to stay away from in. And because users are using it, it limits design possibilities due to the fact that it may cause disruption once it is implemented. If we can just get the word out to not use in, in the future we can define it as we wish, and users can start utilizing it when its ready.

@JinShil
Copy link
Contributor Author

JinShil commented Aug 31, 2019

Should the message also be emitted for disabled code?

I argue it should, because the goal here is to inform the community that the future of in is uncertain, and to stop using it altogether until it's ready.

@rainers
Copy link
Member

rainers commented Aug 31, 2019

I argue it should, because the goal here is to inform the community that the future of in is uncertain, and to stop using it altogether until it's ready.

I was thinking about projects that try to support a range of compiler versions using conditional compilation for constructs that work differently across different versions. in can probably still be replaced by const for older versions without semantic change, though.

@JinShil JinShil changed the title Add -transition=in compiler flag to help users transition away from using in [WIP] Add -transition=in compiler flag to help users transition away from using in Sep 1, 2019
@dlang-bot dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Sep 1, 2019
Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

My objection still stands: We should leave it as is before we have a proper plan for it. Using transition means we are going to change something, in a way that can't be easily deprecated. Here we don't have a plan of timeline for it.

I was not a fan of changing every instance in Phobos & druntime, because it felt like just noise, but it's still things under our control so it matters less than affecting all the D code out there.

@JinShil
Copy link
Contributor Author

JinShil commented Sep 1, 2019

  • Changed the message from deprecation to warning
  • Moved the logic to semantic so only the module being compiled emits messages.
  • A more verbose supplemental message is displayed once for the first occurrence of in and omitted for any additional occurrences.

I still think this is the right thing to do to avoid sabotaging users and to allow designers the liberty to design and implement in right without having to make compromises due to potential disruption.

@JinShil JinShil changed the title [WIP] Add -transition=in compiler flag to help users transition away from using in Add -transition=in compiler flag to help users transition away from using in Sep 1, 2019
@JinShil
Copy link
Contributor Author

JinShil commented Sep 1, 2019

No longer WIP, and ready to go.

@CyberShadow CyberShadow added Review:Needs Approval and removed Review:WIP Work In Progress - not ready for review or pulling labels Sep 2, 2019
@atilaneves
Copy link
Contributor

My objection still stands: We should leave it as is before we have a proper plan for it. Using transition means we are going to change something, in a way that can't be easily deprecated. Here we don't have a plan of timeline for it.

I was not a fan of changing every instance in Phobos & druntime, because it felt like just noise, but it's still things under our control so it matters less than affecting all the D code out there.

There's a plan for it. The plan is to make in mean const scope as intended when dip1000 is enabled. I was working on this last week, I just didn't manage to figure out exactly how to change the compiler to do it in the time I had for the task.

@JinShil
Copy link
Contributor Author

JinShil commented Sep 2, 2019

The plan is to make in mean const scope as intended when dip1000 is enabled.

Sounds good to me.

@JinShil JinShil closed this Sep 2, 2019
@Geod24
Copy link
Member

Geod24 commented Sep 2, 2019

There's a plan for it.

Then there's no need to discourage its usage, instead just provide a transitional switch to make it means what it was supposed to mean. Or pack it with dip1000, whatever you think is best.
And what happens to all the places where it was removed ? Should we revert those ?

@JinShil JinShil deleted the transition_deprecatein branch September 2, 2019 09:47
@atilaneves
Copy link
Contributor

There's a plan for it.

Then there's no need to discourage its usage,

Precisely.

instead just provide a transitional switch to make it means what it was supposed to mean. Or pack it with dip1000, whatever you think is best.

The plan is to pack it in with dip1000.

And what happens to all the places where it was removed ? Should we revert those ?

I didn't think they should have been removed at the time, but I had to talk to Walter first to know if he'd agree with my plan.

@JinShil
Copy link
Contributor Author

JinShil commented Sep 2, 2019

@atilaneves Once in becomes scope const, some things will not compile. See dlang/druntime#2685 (comment) and https://issues.dlang.org/show_bug.cgi?id=20087 for a couple of examples.

@atilaneves
Copy link
Contributor

@JinShil Yes, but only with DIP1000, which is optional and creates a time period for not breaking. Do you have any suggestions on how to handle those two breakages?

@JinShil
Copy link
Contributor Author

JinShil commented Sep 2, 2019

Do you have any suggestions on how to handle those two breakages?

I don't know what to do about dlang/druntime#2685 (comment) yet, but perhaps you could get @WalterBright on the horn and ask him if #10223 could be a solution to Issue 20087.

I also might be able to do the implementation of in for you, but I might need a couple of weeks. I was always willing to do the work, I just needed a decision.

@JinShil
Copy link
Contributor Author

JinShil commented Sep 2, 2019

@atilaneves Also, this is another situation that doesn't build. If you could tell me how the compiler should behave, I might be able to fix it.

@PetarKirov
Copy link
Member

PetarKirov commented Sep 2, 2019

@atilaneves can we also have void foo(in T x) mean void foo(auto ref scope const(T) x) with -preview=dip1000 and -preview=rvaluerefparam? That way in parameters will be the logic counterpart of out parameters. The non-template auto ref meaning pass small types by value and large types by reference, unlike the existing auto ref which takes lvalues always by ref and rvalues by value.

See also:

@atilaneves
Copy link
Contributor

@ZombineDev I'd have to think about that. A lot. I assume by in T x you mean in a template function? I clicked on the two links but am unsure of what exactly is being proposed.

@Geod24
Copy link
Member

Geod24 commented Sep 3, 2019

struct Huge { /* ... */ }
void foo(in Huge val); // This is equivalent to `foo(const scope ref Huge val)`
void foo (in ushort);    // This is equivalent to `foo(const scope ushort)`

Basically, if you're passing an in parameter, it's an input to the function which will only be valid for the duration of the call and we should guarantee that copy don't happen.

@PetarKirov
Copy link
Member

PetarKirov commented Sep 4, 2019

I'd have to think about that. A lot. I assume by in T x you mean in a template function?

Nope, I meant both template and non-template functions. Currently auto ref is a strange thing. You can use it only with template functions - e.g.void foo(T)(auto ref T x).
What's even stranger is that you can't instantiate such templates explicitly, e.g. auto fptr = &foo!T doesn't work. That's because the only way to instantiate a function template with auto ref parameters is by actually calling the function with some arguments. That's because the compiler needs to instantiate either the ref or non-ref version depending on the lvalue-ness of the run-time arguments provided at the call site.
For the same reason, you can't have virtual methods that take auto ref parameters, nor you can define function pointer or delegate types ("callbacks") that accept arguments by auto ref.

What I propose is (I hope) much simpler.
The in parameter attribute in a non-template function void bar(in T x) should be replaced during semantic analysis by either scope const(T), or scope ref const(T), depending on what is the most efficient way of passing the argument of type T. Small POD types should be copied and large types should be passed by reference. It should accept lvalues and rvalues alike (that's the whole point of the -preview=rvaluerefparam flag).

You would be able to define both template and non-template functions, virtual methods, function pointer and delegate types, etc. just like you can today.

In short, I think that auto ref is a dirty and inefficient hack, and with my proposal I hope we would get a much better alternative.

Note: It's technically not a full replacement because I've seen people mutate auto ref parameters and obviously in won't support that. But that's rare enough (and there's even a dscanner check for that), that I don't think it's a big deal.

@Geod24
Copy link
Member

Geod24 commented Sep 4, 2019

Interestingly I had the exact same discussion with @JinShil a few weeks ago. Should there be a DIP for that ?

@atilaneves
Copy link
Contributor

struct Huge { /* ... */ }
void foo(in Huge val); // This is equivalent to `foo(const scope ref Huge val)`
void foo (in ushort);    // This is equivalent to `foo(const scope ushort)`

Basically, if you're passing an in parameter, it's an input to the function which will only be valid for the duration of the call and we should guarantee that copy don't happen.

I think that a copy happening or not is orthogonal to it being an in parameter. One could achieve the same thing as now by labeling that parameter as in ref, assuming in gets implemented as intended.

@atilaneves
Copy link
Contributor

@Geod24 Yes, I think a DIP would be needed.

@PetarKirov
Copy link
Member

PetarKirov commented Nov 1, 2019

@atilaneves

I think that a copy happening or not is orthogonal to it being an in parameter. One could achieve the same thing as now by labeling that parameter as in ref, assuming in gets implemented as intended.

You could apply the same line of reasoning in order to ask for in to be removed from the language (it's just a shorthand for const or scope const, right?!). However I think you'll agree that the main purpose of in is not technical, but social: to help push developers into writing const scope code by default (not impure and mutable by default).
Just like const T& is a solid default for parameters in the C++ community, I think most would agree that scope const ref (with -preview=rvaluerefparam) is at least equally good choice in D. If in's only purpose is to safe a few characters, why not go all the way and make it cover the most common use case?

Also this part:

One could achieve the same thing as now by labeling that parameter as in ref, assuming in gets implemented as intended.

Is not true, because we would also need to change the semantics of scope const ref to mean @auto_ref scope const unconditionally, which I am not proposing, because I think there's value in having finer and more-orthogonal control by using this more explicit form.

  • scope const T - passes arguments always by value.
  • ref scope const T - passes arguments always by reference.
  • in T -> @auto_ref scope const T - passes arguments in the most efficient way (depending on the whether T is POD type and its size).

(I use @auto_ref mean pass args in the most efficient way; unlike the current auto ref which is usable only for template functions which works depending not on the efficiency, but depending on whether the arg is lvalue vs rvalue.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants