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 Issue 21885 - Bad diagnostic: struct is not copyable because it is annotated @disable #12513

Merged
merged 1 commit into from
May 23, 2021

Conversation

RazvanN7
Copy link
Contributor

@RazvanN7 RazvanN7 commented May 11, 2021

I had to fix a few tests, but IMHO the errors are more accurate now.

In case a struct has a generated postblit that is disabled, it means that the struct has a field that has a disabled postblit. The patch searches for the first field that has a disabled postblit and outputs it. This code is similar to the one in buildPostblit, therefore an alternative solution could store the fields with disabled postblits when the postblit is constructed, however, I think it's better to redo the work since we are on a failing branch.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @RazvanN7! 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
21885 enhancement Bad diagnostic: struct is not copyable because it is annotated @disable

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

/*
TEST_OUTPUT:
---
fail_compilation/fail21885.d(24): Error: struct `fail21885.Outer` is not copyable because field `i` is not copyable
Copy link
Member

Choose a reason for hiding this comment

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

Can we recurse, so that "because it has a disabled postblit" is shown as supplemental ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't understand what you mean. Do you expect here to have:

Error: struct `fail21885.Outer` is not copyable because it has a disabled postblit.
          The postblit is disabled because field `i' is not copyable and no user defined postblit was provided.

?

Copy link
Member

Choose a reason for hiding this comment

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

struct A { B b; }
struct B { C c; }
struct C { int a; @disable this(this); }
void foo (A a);

I mean that this should produce, if possible:

file.d:42: Error: struct `mod.A` is not copyable because field `b` is not copyable.
                           struct `mod.B` is not copyable because field `c` is not copyable
                           struct `mod.C` is not copyable because it has a disabled postblit

Copy link
Contributor Author

@RazvanN7 RazvanN7 May 11, 2021

Choose a reason for hiding this comment

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

I see. That would be easy to implement, but I think that this could be implemented behind a switch as for large structs the output could be quite verbose. Anyway, I would rather get this in and file your proposal as an enhancement request.

fail_compilation/fail10968.d(77): Error: struct `fail10968.SD` is not copyable because it is annotated with `@disable`
fail_compilation/fail10968.d(78): Error: struct `fail10968.SD` is not copyable because it is annotated with `@disable`
fail_compilation/fail10968.d(79): Error: struct `fail10968.SD` is not copyable because it is annotated with `@disable`
fail_compilation/fail10968.d(72): Error: struct `fail10968.SD` is not copyable because it has a disabled postblit
Copy link
Contributor

@nordlow nordlow May 12, 2021

Choose a reason for hiding this comment

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

I would prefer having an errorSupplemental printing

"disabled postblit is defined here"....

I guess that could be added postponed to @Geod24's enhancement request.

@RazvanN7 RazvanN7 requested a review from thewilsonator May 19, 2021 07:15
@RazvanN7 RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label May 20, 2021
@thewilsonator thewilsonator merged commit 7f4620f into dlang:master May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Review:Easy Review Severity:Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants