Skip to content

Conversation

@JinShil
Copy link
Contributor

@JinShil JinShil commented Jul 15, 2019

An alternative strategy to #10168

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

@JinShil JinShil changed the title Temporarily discoure the use of as a parameter storage class Temporarily discourage the use of as a parameter storage class Jul 15, 2019
@JinShil JinShil changed the title Temporarily discourage the use of as a parameter storage class Temporarily discourage the use of in as a parameter storage class Jul 15, 2019
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 to me, but I guess this needs approval from one of the captains.

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.

I'm strongly opposed to any "temporary discouragement" as mentioned in the linked PR.
Either we fix it or we kill it.

@JinShil
Copy link
Contributor Author

JinShil commented Jul 15, 2019

Either we fix it or we kill it.

I'm OK with fixing it today. I'm only pursuing the temporary warning as I believe that is what is expected of me.

@CyberShadow
Copy link
Member

This is insanity. If this compiler change requires so many changes in Phobos, what about all the rest of D code? There is no way such a breaking change could be accepted. The Phobos/Druntime PRs are just noise and making the code and declarations harder to read.

Was this change + all the necessary Phobos and Druntime changes approved by Walter and Andrei?

@JinShil
Copy link
Contributor Author

JinShil commented Jul 29, 2019

This is insanity

That's exaggeration and FUD.

in is broken. The best way forward, so at some time in the future a fix can be properly designed and implemented without causing widespread breakage, is to reduce its usage. We may not accept this PR, but we should be discouraging its use.

T`he Phobos/Druntime PRs are just noise and making the code and declarations harder to read.

Once again more exaggeration and FUD. If you're using in your code is wrong because in is not properly implemented. The Phobos and druntime PRs fixed the code. If in is ever implemented properly, the code will still work and be correct. It is a better path for the future.

Walter and Andrei

If only Walter and Andrei would render an opinion on everything we do. If you want to have a say, get involved.

@CyberShadow
Copy link
Member

You can't fix a language by breaking all the code written in it. You're not fixing anything, you're just breaking the world.

If you're using in your code is wrong because in is not properly implemented.

If it's actually in widespread use, it doesn't matter what the documentation says or what you think. Large-scale code breakage is simply not an option.

If you want to have a say, get involved.

This is me getting involved.

@JinShil
Copy link
Contributor Author

JinShil commented Jul 29, 2019

You can't fix a language by breaking all the code written in it

How is changing in to scope const breaking anything? That's what it's supposed to be.

This is me getting involved.

You're late.

@CyberShadow
Copy link
Member

CyberShadow commented Jul 29, 2019

How is changing in to scope const breaking anything? That's what it's supposed to be.

  1. What anything is "supposed to be" is completely irrelevant when discussing code breakage. There simply cannot exist any possible justification for massive code breakage.
  2. The problem is not the Phobos changes, it's this compiler change which requires them, which AIUI is very likely to require extensive changes in users' D code as well.

You're late.

No, we can still discuss merging this PR, and revert the Phobos/Druntime PRs.

@CyberShadow
Copy link
Member

If you want to change the meaning of in, why not do this in the same way that the meaning of unary ~ was fixed? I.e. emit a deprecation warning only when it's used in circumstances where the result would be potentially different according to the old and new meaning.

@JinShil
Copy link
Contributor Author

JinShil commented Jul 29, 2019

The problem is not the Phobos changes, it's this compiler change which requires them, which AIUI is very likely to require extensive changes in users' D code as well.

Fair enough. There are other alternatives to this PR.

  1. in is already being discouraged in the documentation, but it's likely that the documentation is not being read by the masses
  2. I can create a running changelog entry for a year or so warning users that they should no longer use in and are encouraged to change their code. (i.e. no changes to the compiler)
  3. I can create a rule in DScanner and let that simmer for a year or more.

Bottom line is we don't need to merge this PR. @Geod24 already voiced opposition to it, and I'm considering the above alternatives to reduce any potential anxiety.

I.e. emit a deprecation warning only when it's used in circumstances where the result would be potentially different according to the old and new meaning.

Sounds interesting. I'm not sure how that can be implemented. Please share more details and I'll consider doing the work.

@CyberShadow
Copy link
Member

  • in is already being discouraged in the documentation, but it's likely that the documentation is not being read by the masses

  • I can create a running changelog entry for a year or so warning users that they should no longer use in and are encouraged to change their code.

  • I can create a rule in DScanner and let that simmer for a year or more.

I think these are good aids but might be insufficient by themselves.

I had a look at e.g. vibe.d, and that has roughly 200 instances of using in in function signatures.

Sounds interesting. I'm not sure how that can be implemented. Please share more details and I'll consider doing the work.

Here is the implementation for the ~ transition:

dmd/src/dmd/dcast.d

Lines 3552 to 3586 in b25b676

/******************************************************
* This provides a transition from the non-promoting behavior
* of unary + - ~ to the C-like integral promotion behavior.
* Params:
* sc = context
* ue = NegExp, UAddExp, or ComExp which is revised per rules
* References:
* https://issues.dlang.org/show_bug.cgi?id=16997
*/
void fix16997(Scope* sc, UnaExp ue)
{
if (global.params.fix16997)
ue.e1 = integralPromotions(ue.e1, sc); // desired C-like behavor
else
{
switch (ue.e1.type.toBasetype.ty)
{
case Tint8:
case Tuns8:
case Tint16:
case Tuns16:
//case Tbool: // these operations aren't allowed on bool anyway
case Tchar:
case Twchar:
case Tdchar:
ue.deprecation("integral promotion not done for `%s`, use '-preview=intpromote' switch or `%scast(int)(%s)`",
ue.toChars(), Token.toChars(ue.op), ue.e1.toChars());
break;
default:
break;
}
}
}

If you're stuck, Walter and Andrei are generally responsive to emails. (Andrei would be the person to consult about design / semantics / corner cases, and Walter about compiler internals.)

@JinShil
Copy link
Contributor Author

JinShil commented Jul 29, 2019

I think these are good aids but might be insufficient by themselves.

I can also submit PRs to many prominent repositories. The change is usually very simple and just takes a few minutes. I don't mind picking away at it gradually over the course of a couple years if it will free us from this technical debt.

Here is the implementation for the ~ transition:

I understand the general idea, but how does one distinguish between cases where it matters and cases where it doesn't. It seems to me it requires data-flow analysis or the user to state their intent.

If you're stuck, Walter and Andrei are generally responsive to emails.

Yes, they are, but they've stated to me a couple of times that they can't be bothered with such things. I'm forced to assume that they are quite content with the warts, technical debt, and other weird sh** in the language, and it's therefore on me if I want something done about it.

@CyberShadow
Copy link
Member

I understand the general idea, but how does one distinguish between cases where it matters and cases where it doesn't. It seems to me it requires data-flow analysis or the user to state their intent.

That may be true, but I don't know how you intend to redefine in to comment. If an exact check is impractical, a heuristic with false positives would be OK too.

Yes, they are, but they've stated to me a couple of times that they can't be bothered with such things.

This must have been a miscommunication. It's just not possible for Walter and Andrei to not be bothered about core language changes.

@JinShil
Copy link
Contributor Author

JinShil commented Jul 30, 2019

It appears that Issue 16997 was addressed by creating a transition flag that reverted the compiler to the old, incorrect behavior. If the flag was not used, the compiler would exhibit the correct behavior, breaking code.

I could implement a transition flag that, when used, reverts in to be just a synonym for const as it is today. If the transition flag is not used, then in would be a synonym for const scope. That could break code, forcing users to use the flag to revert to the older, currently incorrect, behavior to avoid having to modify their code.

Is that what you're suggesting?

@CyberShadow
Copy link
Member

It appears that Issue 16997 was addressed by creating a transition flag that reverted the compiler to the old, incorrect behavior. If the flag was not used, the compiler would implement the correct behavior, breaking code.

I would avoid using the terms "correct" / "incorrect" here. If a program relied on the old behavior and was known to work correctly, then it would be inappropriate to call the old behavior "incorrect", as "fixing" it would break the program.

I'm not sure about the transition flag. Aside the -preview=XXX, we have -transition=XXX, which only emit warnings without changing behavior. I think the more important aspect of that approach was how it activated only when the operator was applied to small types (not 32 or 64-bit integers, for which the behavior remained unchanged).

@JinShil
Copy link
Contributor Author

JinShil commented Jul 30, 2019

I think the more important aspect of that approach was how it activated only when the operator was applied to small types (not 32 or 64-bit integers, for which the behavior remained unchanged).

I believe I can limit the deprecation warning to parameters that are passed by reference. Then I can add a -revert=in_is_const_scope switch that users can utilize to revert to "old" behavior (i.e. in is const) and silence the deprecation warning. How's that?

@CyberShadow
Copy link
Member

I think -revert (opt-out) was preceded by an opt-in flag (-dip25) for a number of releases. This sounds like a good idea, to ensure that the new implementation behaves correctly in unexpected circumstances that users might use it in.

@JinShil
Copy link
Contributor Author

JinShil commented Jul 30, 2019

I think -revert (opt-out) was preceded by an opt-in flag (-dip25) for a number of releases.

Ok, then what about this:

  1. I implement a -preview=deprecate_in switch that will enable the deprecation message for a year. A changelog entry about it will raise awareness.
  2. After 1 year -preview=deprecate_in becomes the default, and -revert=deprecated_in is added to revert to the old (i.e. in is const) behavior. -revert=deprecate_in will remain at least for 1 year, maybe even indefinitely depending the D ecosystem reacts.

I'm recommending a deprecation of in because I had a discussion with @Geod24 that we may want to redefine in in the future to actually be const scope ref for any non-reference type. I think that will require a DIP, but deprecating in will at least decrease its usage so that such a DIP could, someday, be a realistic possibility.

I could also just implement in as scope const instead of depreciating it.

I would actually prefer in being scope const ref because it is more analogous with out. But mostly I just want unimplemented features, poorly implemented features, and other "weird sh**" in D fixed. I leave it to you all to decide.

@CyberShadow
Copy link
Member

CyberShadow commented Jul 31, 2019

  • I implement a -preview=deprecate_in switch that will enable the deprecation message for a year. A changelog entry about it will raise awareness.

  • After 1 year -preview=deprecate_in becomes the default, and -revert=deprecated_in is added to revert to the old (i.e. in is const) behavior. -revert=deprecate_in will remain at least for 1 year, maybe even indefinitely depending the D ecosystem reacts.

That sounds good assuming the entire plan is met with approval in general.

I'm recommending a deprecation of in because I had a discussion with @Geod24 that we may want to redefine in in the future to actually be const scope ref for any non-reference type. I think that will require a DIP, but deprecating in will at least decrease its usage so that such a DIP could, someday, be a realistic possibility.

Honestly I think you would have a much better chance of going forward with deprecating in if you already have a good plan prepared with a replacement, instead of the other way around. If it turns out that figuring out new semantics for in or implementing them in DMD turns out to be too difficult, or that the problems they would solve turn out to be better solved through other ways, then we will have broken and uglified all that D code for nothing.

I leave it to you all to decide.

Honestly I'm really confused why it has to be me who has to carry out this conversation. There are many people in our community who have a much better understanding of the compiler internals and language semantics than myself, starting with Walter and Andrei who are at the top of the language design responsibility chain, and it bothers me that they all are conspicuously absent in this conversation, and also that everyone seemed to be perfectly OK with all the Phobos/Druntime PRs even though the final destination for which they were created is still unclear. Maybe we're conducting the conversation in the wrong place?

@wilzbach
Copy link
Contributor

Walter and Andrei who are at the top of the language design responsibility chain

Andrei stepped down and @atilaneves is now in charge.

Maybe we're conducting the conversation in the wrong place?

It's not the first time this came up on other channels. Never led to any results there.

perfectly OK with all the Phobos/Druntime PRs even though the final destination for which they were created is still unclear.

Walter even did some of these PRs himself a year ago for Phobos when he needed this for DIP 1000. The point was that in never meant scope, so these PRs were definitely a step forward.

@wilzbach
Copy link
Contributor

Honestly I'm really confused why it has to be me who has to carry out this conversation.

Because everyone else stopped bothering?
I personally lost interest in this topic since:

#8021

So I'm very happy that Mike tries to fix the status quo.

going forward with deprecating in if you already have a good plan prepared with a replacement, instead of the other way around

in is pretty much equal to const and thus defunct. Whatever happens it needs to be deprecated first. Even if we decide to entirely drop it or go with a different approach, a deprecation of a defunct feature is very welcome.

@CyberShadow
Copy link
Member

a deprecation of a defunct feature is very welcome

Maybe welcome to people who don't maintain a hundred thousand lines of D code across dozens of projects and services, but certainly not me! We need to balance the utility and cost of all changes always. Completely deprecating in without a clear plan forward for a replacement seems much more costly than useful to me right now.

@Geod24
Copy link
Member

Geod24 commented Jul 31, 2019

Honestly I think you would have a much better chance of going forward with deprecating in if you already have a good plan prepared with a replacement, instead of the other way around. If it turns out that figuring out new semantics for in or implementing them in DMD turns out to be too difficult, or that the problems they would solve turn out to be better solved through other ways, then we will have broken and uglified all that D code for nothing.

For reference, this is what I proposed. We should leave in be for the time being, unless we have a proper replacement. Personally I suggested an idea to @JinShil but I currently don't have time to dedicate to writing this DIP / implementation.

@JinShil
Copy link
Contributor Author

JinShil commented Jul 31, 2019

Honestly I think you would have a much better chance of going forward with deprecating in if you already have a good plan prepared with a replacement, instead of the other way around. If it turns out that figuring out new semantics for in or implementing them in DMD turns out to be too difficult, or that the problems they would solve turn out to be better solved through other ways, then we will have broken and uglified all that D code for nothing.

Replacing in with scope and scope const in existing code (the druntime and Phobos PRs), is completely benign, and has only been positive. It has uncovered a few existing bugs (that were hiding behind in) which are gradually being addressed, and regardless of what ultimately happens with in, the code will NOW no longer be at any risk of breakage. We need to be doing more such changes so when a decision is finally made about in, the consequences of that decision will have minimal impact. The druntime and Phobos changes haven't been, and will not have been, for nothing.

This will also be positive for the language designers and decision-makers. If, and when, a DIP is created, or other decision made about in, the designer will have the liberty to design without fear of causing much disruption or having to make too many compromises that could handicap the feature and/or the language.

I will implement a -preview=deprecatein switch that will be opt-in. With an accompanying changelog entry, we can raise awareness in the community that users are taking a risk if they use in. Any users with existing code will have what they need to identify usages of in and update their code to maintain the SAME semantics as they have today, but, in addition, protect themselves from the risk of future breakage. No more booby traps like we have today.

We don't need to put a time limit on the new compiler switch, or eventually make the deprecation an error (unless that's what's decide at a later date). What I'm proposing is the equivalent of a public service announcement to help users write more stable, future-proof D code, and provide them with a tool to help them update their existing code, so they can avoid the booby traps that we've currently created for them with regard to in. It'll be entirely opt-in, so will not break anyone's code. After I've finished updating druntime and Phobos we can turn the switch on in the build scripts to be sure the code remains stable.

Please don't worry about the disruption. I've heard you and I'll make the above accommodations. Please just give me your support by merging my druntime and Phobos PRs so that I can finish the task. Once done, all this controversy surrounding in can be eventually put behind us, and the language designers and decision-makers can have the liberty to do a proper design in the future.

@atilaneves
Copy link
Contributor

I'm currently discussing the future of in with Walter.

@12345swordy
Copy link
Contributor

12345swordy commented Jul 31, 2019

Why can't in simply be redefined as const scope?

@DavidBennettPIO
Copy link

DavidBennettPIO commented Jul 31, 2019

Why can't in simply be redefined as const scope?

I also would prefer this option, but as a compromise would it be possible to have in as const scope but if it fails to compile, change it to const and show the errors as deprecation's?

@PetarKirov
Copy link
Member

@atilaneves Any updates?

@12345swordy
Copy link
Contributor

@WalterBright @atilaneves ping

@JinShil
Copy link
Contributor Author

JinShil commented Sep 1, 2019

Superseded by #10382

@JinShil JinShil closed this Sep 1, 2019
@atilaneves
Copy link
Contributor

I somehow missed the pings on this. I replied on #10382

@JinShil JinShil deleted the discourage_in branch September 2, 2019 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants