dmd icon indicating copy to clipboard operation
dmd copied to clipboard

Implement DIP1029: Add `throw` as Function Attribute

Open ljmf00 opened this issue 3 years ago • 24 comments

https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1029.md

Signed-off-by: Luís Ferreira [email protected]


  • [x] Specification PR (https://github.com/dlang/dlang.org/pull/3082)
  • [x] DMD Implementation
  • [x] Tests
    • [x] Header generation of throw
    • [x] Templated functions: do they infer default or throw? how does that interact with toplevel throw/nothrow?
    • [ ] throw virtual functions that are overriden with nothrow
    • [x] more cases for throw:/nothrow: interacting with function/template declarations
    • [x] Add tests with __traits(getFunctionAttributes, func)
    • [ ] Cast nothrow to throw
    • [ ] Check for forbidden cast of throw to nothrow

CC: @WalterBright

ljmf00 avatar Jul 29 '21 02:07 ljmf00

Thanks for your pull request, @ljmf00!

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

dlang-bot avatar Jul 29 '21 02:07 dlang-bot

Buildkite seems to fail due to some side effects, not related to this PR. Although, need to write some more tests to cover all this DIP. For now, I would appreciate a review of my implementation approach. This needs an autosquash rebase on my side to get rid of fixup commits. My initial approach doesn't work as it introduces a breaking change and it also produces a wrong behaviour. The fixup creates a "default" behaviour which is not having either throw or nothrow, similar to @safe, @trusted and @system approaches, where no attribute means @system.

ljmf00 avatar Jul 30 '21 02:07 ljmf00

This still requires some tests for:

  • header generation of throw
  • templated functions: do they infer default or throw? how does that interact with toplevel throw/nothrow?
  • throw virtual functions that are overriden with nothrow
  • more cases for throw:/nothrow: interacting with function/template declarations

Thanks for the list :+1: . I'm going to do that.

The DIP does not specify whether inference should deduce throw or default for functions that are not nothrow. I would argue that a function has to be throw or nothrow; default is just an intermediary state where the compiler does not know but should assume throw. As a precedent, in the case of safety, @system is specifically deduced when inference occurs. In this implementation there seems to be a gap between throw and default. Looking at test/compilable/dip1029.d line 41 it seems that inference does not deduce throw, but default. This is important for things like header generation, error messages, typeof etc.

I choose to use a default mechanism to prevent breaking changes.

I thought that this was not specified in the DIP because it is implicitly implied by "no breaking changes". It is important to test this behaviour because existing code might get the typeof a throw function without specifying any attribute and adding throw attribute to the returned type may introduce a breaking change.

void foo() {}
void bar() @system {}

Doing a typeof of foo and bar produces different output and if the user compares it with, for example, a .stringof, this may break it.

I also see this as acceptable because this is how the @system attribute comparison works with the default value.

is(void function() == void function() @system) // true

AFAIK, the intermediate state where the compiler can't assume throw is validated by FUNCFLAG.nothrowInprocess rather than THROW.default_.

Tell me if I interpreted your comment/suggestion wrongly.

ljmf00 avatar Aug 06 '21 10:08 ljmf00

Doing a typeof of foo and bar produces different output and if the user compares it with, for example, a .stringof, this may break it.

In your example there is no inference. In that case, I agree that typeof should return whatever the explicit annotation is. However, in the case of delegates/lambdas it's not that obvious: these always have a body and their attributes are explicitly embedded in the type (the same as @system).

Also, for template functions, I think that throw should be explicitly added as it is the case for system however that might break some code that uses .stringof the way you mentioned.

To summarize: should function declarations that have their attributes inferred (deletgates, lambdas, function templates) explicitly be marked as throw or be left unmarked? On the long run, the compiler should explicitly annotate with throw, however, this might break some code (when generating headers that contain delegates/lambdas, when checking the type of template instances via .stringof etc.)

cc @WalterBright

RazvanN7 avatar Aug 06 '21 10:08 RazvanN7

Doing a typeof of foo and bar produces different output and if the user compares it with, for example, a .stringof, this may break it.

In your example there is no inference. In that case, I agree that typeof should return whatever the explicit annotation is. However, in the case of delegates/lambdas it's not that obvious: these always have a body and their attributes are explicitly embedded in the type (the same as @system).

Also, for template functions, I think that throw should be explicitly added as it is the case for system however that might break some code that uses .stringof the way you mentioned.

To summarize: should function declarations that have their attributes inferred (deletgates, lambdas, function templates) explicitly be marked as throw or be left unmarked? On the long run, the compiler should explicitly annotate with throw, however, this might break some code (when generating headers that contain delegates/lambdas, when checking the type of template instances via .stringof etc.)

I agree. We could add a -preview=dip1029 to express that behaviour. I'm not aware of the preview process, but I assume is the same as the deprecation process but inverted, right?

ljmf00 avatar Aug 06 '21 11:08 ljmf00

This code will still have breaking changes. __traits(getFunctionAttributes) gives you @system on a function without ~~parameters~~ attributes.

ljmf00 avatar Aug 06 '21 12:08 ljmf00

however that might break some code that uses .stringof the way you mentioned.

As far as I know the language spec makes no guarantees whatsoever about the specific contents of .stringof, so anyone who relies on it in this way is doing so at their own risk.

pbackus avatar Aug 25 '21 13:08 pbackus

Needs #13121 for trailing whitespace free tests.

ljmf00 avatar Oct 03 '21 19:10 ljmf00

#13121 has been merged.

thewilsonator avatar Oct 06 '21 09:10 thewilsonator

#13121 has been merged.

Yes, but I still need to commit the header generation tests and fix the logic for it.

ljmf00 avatar Oct 06 '21 13:10 ljmf00

I added the rest of the test cases. I just need to add cast() and virtual functions.

This code will still have breaking changes. __traits(getFunctionAttributes) gives you @system on a function without ~parameters~ attributes.

As I suspected, this fails to compile on Phobos. Should I add a flag to the compiler to make the transition or make __traits(getFunctionAttributes) hide throw on THROW.default_? That will make it a special case tho. How can we proceed here @RazvanN7 ?

ljmf00 avatar Feb 08 '22 00:02 ljmf00

We have the following solutions:

  • omit throw when it is not specifically added by the user. This would be the easiest way, however, it will be inconsistent with what happens in the case of @system/@safe.
  • issue a deprecation whenever getFunctionAttributes is used on a throw function that is not annotated as such by the user.
  • fix the breakages and go with it as is.

Either way we choose it's a bit nasty. I, personally, would be inclined to go the latter route since I expect that the way phobos uses getFunctionAttributes is not a common occurrence. But, I think that there are folks who might not necessarily agree with me, so issuing a deprecation would be a safer alternative, however this is problematic because if you are reflecting on library code there is no way to get rid of the deprecation.

RazvanN7 avatar Feb 08 '22 08:02 RazvanN7

Another alternative would be to deprecate getFunctionAttributes altogether for a newer getFuncAttrs which outputs throw, however, this is not scalable if we intend to implement impure and gc

RazvanN7 avatar Feb 08 '22 08:02 RazvanN7

  1. Would this be a precursor for diagnostics akin to: error: func() throws but is not caught
  2. Possibility of future extensions to allow specifying which exception is thrown, e.g: int openFile() throw(FileException) { ... }

ibuclaw avatar Feb 08 '22 10:02 ibuclaw

  1. Would this be a precursor for diagnostics akin to: error: func() throws but is not caught

I don't think so, the default, which is the "hidden" throw doesn't behave like that, why would with specified throw? I would argue that makes sense if we specified the Exception class, explicitly, but this DIP doesn't cover that.

  1. Possibility of future extensions to allow specifying which exception is thrown, e.g: int openFile() throw(FileException) { ... }

This DIP only creates a contrary attribute to negate nothrow.

ljmf00 avatar Feb 08 '22 18:02 ljmf00

fix the breakages and go with it as is.

I can create an attempt to fix the breakage on Phobos, but that doesn't guarantee the fact that this will break user code, although the user should be prepared for those changes, as any other attribute might get introduced. To do this attempt, should I add a synchronization/transition flag to do that transition? If yes, what flag? My suggestion would be to stick with a -revert= flag, that way users can revert that behaviour to the old one, preventing breakage and in the long term, deprecate that flag when we have impure and @gc?

ljmf00 avatar Feb 08 '22 19:02 ljmf00

Ping @RazvanN7 :)

ljmf00 avatar Feb 28 '22 20:02 ljmf00

The more I think about this, the more I feel that a deprecation is a more suitable alternative. We added a new function attribute, people need to update their code to take it into account.

RazvanN7 avatar Mar 01 '22 09:03 RazvanN7

The more I think about this, the more I feel that a deprecation is a more suitable alternative. We added a new function attribute, people need to update their code to take it into account.

I can agree with the deprecation if we implement this to the other attributes, I would say.

ljmf00 avatar Mar 01 '22 16:03 ljmf00

Yes, definitely, if other attributes will be implemented then the same deprecation path must be taken.

RazvanN7 avatar Mar 08 '22 09:03 RazvanN7

Yes, definitely, if other attributes will be implemented then the same deprecation path must be taken.

Ah, so you mean this?

  • issue a deprecation whenever getFunctionAttributes is used on a throw function that is not annotated as such by the user.

I thought you were talking about implementing the new trait and deprecate the older one. But yeah, I share the same opinion, if that's what you mean.

ljmf00 avatar Mar 08 '22 12:03 ljmf00

Ah, so you mean this?

Yes.

RazvanN7 avatar Mar 08 '22 12:03 RazvanN7

Ah, so you mean this?

Yes.

Ok, I can go with implementing it that way. I think this is the best way among the other options we have.

ljmf00 avatar Mar 08 '22 12:03 ljmf00

ping @ljmf00

RazvanN7 avatar Apr 26 '22 07:04 RazvanN7

@ljmf00 any updates on this? This is so close to completion, it's a pity it got stalled so much.

RazvanN7 avatar Sep 23 '22 07:09 RazvanN7