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

Infer noreturn for auto functions #12553

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@dlang-bot
Copy link
Contributor

dlang-bot commented May 21, 2021

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

@MoonlightSentinel
Copy link
Contributor Author

MoonlightSentinel commented May 21, 2021

This PR currently has a negative implication for callbacks as seen in the druntime failure:

void fun(void function() f);
void fun(void delegate() d);

fun(() { throw new Exception(""); });

The call above is now ambiguous because both overloads are considered as equal (MATCH.convert due to noreturn => void).

@MoonlightSentinel
Copy link
Contributor Author

Resolved the ambiguity by upping the matching level for noreturn => void to MATCH.constant. This is valid because both neither noreturn nor void represent actual values and hence are more closely related than other types.

Another possible approach would be to adapt the matching code for function / delegate literals, allthough that wouldn't be as encapsulated as the current approach.


This currently fails because of the dreaded statement not reachable warning in templated code, e.g. for the non-error path (return null) in collectException.

@pbackus
Copy link
Contributor

pbackus commented Oct 5, 2021

Resolved the ambiguity by upping the matching level for noreturn => void to MATCH.constant. This is valid because both neither noreturn nor void represent actual values and hence are more closely related than other types.

Really not a fan of putting special-case hacks like this in the type system. This is the kind of decision that can seem like the lesser of two evils at the time, but that future D programmers will end up cursing us for.

The correct solution is to disambiguate the call in druntime by giving the function literal an explicit return type.

(Arguably, the actual correct solution is to implement implicit conversion of function pointers to delegate pointers, which would eliminate the need for separate overloads in the first place—but that's out of scope for this discussion.)

MoonlightSentinel added a commit to MoonlightSentinel/druntime that referenced this pull request Oct 7, 2021
dlang/dmd#12553 will change the return type inference s.t. these lambdas
are inferred as `noreturn` instead of `void`. This causes an ambiguity
regarding the `void function()` and the `void delegate` overloads.
dlang-bot pushed a commit to dlang/druntime that referenced this pull request Oct 7, 2021
dlang/dmd#12553 will change the return type inference s.t. these lambdas
are inferred as `noreturn` instead of `void`. This causes an ambiguity
regarding the `void function()` and the `void delegate` overloads.
MoonlightSentinel added a commit to MoonlightSentinel/mir-core that referenced this pull request Oct 14, 2021
dlang/dmd#12553 will enable the frontend to infer `noreturn` as the
return type of templated / auto functions.

The unittest fails with this PR because `collideWith(Asteroid, Spaceship)`
always throws an exception and hence is inferred as `noreturn`. This
causes `collide(Asteroid, Spaceship)` to return `string` instead of
`void` because `noreturn` is implicitly convertible to any type.
MoonlightSentinel added a commit to MoonlightSentinel/mir-core that referenced this pull request Oct 14, 2021
dlang/dmd#12553 will enable the frontend to infer `noreturn` as the
return type of templated / auto functions.

The unittest fails with this PR because `collideWith(Asteroid, Spaceship)`
always throws an exception and hence is inferred as `noreturn`. This
causes `collide(Asteroid, Spaceship)` to return `string` instead of
`void` because `noreturn` is implicitly convertible to any type.
9il pushed a commit to libmir/mir-core that referenced this pull request Oct 18, 2021
dlang/dmd#12553 will enable the frontend to infer `noreturn` as the
return type of templated / auto functions.

The unittest fails with this PR because `collideWith(Asteroid, Spaceship)`
always throws an exception and hence is inferred as `noreturn`. This
causes `collide(Asteroid, Spaceship)` to return `string` instead of
`void` because `noreturn` is implicitly convertible to any type.
Infer noreturn whenever the function body is never exited normally
@RazvanN7
Copy link
Contributor

@MoonlightSentinel What is the state of this draft?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants