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

Fix 21538,20904: Correct implicitConvTo order for delegate parameters in Type.covariant #13267

Merged
merged 1 commit into from Nov 13, 2021

Conversation

MoonlightSentinel
Copy link
Contributor

@MoonlightSentinel MoonlightSentinel commented Nov 5, 2021

The ordering was inversed when compared to function pointers. That caused
the check to accept e.g. less restrictive attributes for callbacks.

Also changed the equality checks for TypeFuntion and TypeDelegate
to equals because == didn't call the former and hence caused problems
for duplicate instances.


Preliminary check whether this affects projects on BuildKite as this breaks a test case in runnable/xtest46.d (which is an accept-invalid when using delegate parameters and already rejected for function pointers).

The test case in runnable/xtest46.d was not found in any project on Buildkite

@dlang-bot
Copy link
Contributor

dlang-bot commented Nov 5, 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

Auto-close Bugzilla Severity Description
20904 major dip1000 implicit conversion delegates error
21538 regression Overriding with more attributes on delegate parameter is allowed

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

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

@thewilsonator
Copy link
Contributor

Should this target stable?

MoonlightSentinel added a commit to MoonlightSentinel/io that referenced this pull request Nov 5, 2021
The overriding functions assumed stricter qualifiers (`@safe` / `scope`)
for the callback that are not guaranteed by the definition in `Throwable`.

The bug was hidden by [this issue](https://issues.dlang.org/show_bug.cgi?id=21538),
see dlang/dmd#13267.
MoonlightSentinel added a commit to MoonlightSentinel/io that referenced this pull request Nov 5, 2021
The overriding functions assumed stricter qualifiers (`@safe` / `scope`)
for the callback that are not guaranteed by the definition in `Throwable`.

The bug was hidden by [this issue](https://issues.dlang.org/show_bug.cgi?id=21538),
see dlang/dmd#13267.
@PetarKirov
Copy link
Member

Should this target stable?

It can't without dropping the unit tests, since it builds on top of: #13238, which was already merged in master.

MoonlightSentinel added a commit to MoonlightSentinel/io that referenced this pull request Nov 5, 2021
The overriding functions assumed stricter qualifiers (`@safe` / `scope`)
for the callback that are not guaranteed by the definition in `Throwable`.

The bug was hidden by [this issue](https://issues.dlang.org/show_bug.cgi?id=21538),
see dlang/dmd#13267.
@MoonlightSentinel
Copy link
Contributor Author

MoonlightSentinel commented Nov 5, 2021

Should this target stable?

Probably

It can't without dropping the unit tests, since it builds on top of: #13238, which was already merged in master.

Could also cherry-pick the commit in this PR

MoonlightSentinel added a commit to MoonlightSentinel/io that referenced this pull request Nov 5, 2021
The overriding functions assumed stricter qualifiers (`@safe` / `scope`)
for the callback that are not guaranteed by the definition in `Throwable`.

The bug was hidden by [this issue](https://issues.dlang.org/show_bug.cgi?id=21538),
see dlang/dmd#13267.
@PetarKirov
Copy link
Member

Even though this a type system bug fix, it's also technically a breaking change, so I suggest master to be on the safe side.

... in `Type.covariant`

The ordering was inversed when compared to function pointers. That caused
the check to accept e.g. less restrictive attributes for callbacks.

Also changed the equality checks for `TypeFuntion` and `TypeDelegate`
to `equals` because `==` didn't call the former and hence caused problems
for duplicate instances.
@RazvanN7 RazvanN7 added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Nov 10, 2021
@MoonlightSentinel MoonlightSentinel added Regression PRs that fix regressions and removed Enhancement labels Nov 10, 2021
@thewilsonator thewilsonator added auto-merge and removed 72h no objection -> merge The PR will be merged if there are no objections raised. labels Nov 13, 2021
@dlang-bot dlang-bot merged commit 32f3bfd into dlang:master Nov 13, 2021
@MoonlightSentinel MoonlightSentinel deleted the covariant-fix branch November 16, 2021 09:54
@ibuclaw
Copy link
Member

ibuclaw commented Dec 20, 2022

The test case in runnable/xtest46.d was not found in any project on Buildkite

However it was found in industry codebases.

Possibly caused a regression. https://issues.dlang.org/show_bug.cgi?id=23274

Though it should be thought carefully about whether the code broken in xtest46.d was indeed wrong in the first place.

@CyberShadow
Copy link
Member

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=23956

Possibly the same issue as the one above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Bug Fix Regression PRs that fix regressions
Projects
None yet
7 participants