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

compiler: be more clear about transparent layout violations #114015

Closed
wants to merge 3 commits into from

Conversation

dvdhrm
Copy link
Contributor

@dvdhrm dvdhrm commented Jul 24, 2023

Extend the error-messages of rustc to distinguish between non-zero-sized fields and fields without layout when annotating repr(transparent) violations.

When compiling repr(transparent) structures, the compiler checks that there is at most one field with non-zero size OR having unknown layout. However, the error message treats both conditions as the same, thus always annotating them as;

a: u32,
------ this field is non-zero-sized

In most situations, this is reasonable. However, if you consider generic fields, this can easily become confusing. Consider the following error message:

a: [T; 0],
--------- this field is non-zero-sized

This field is clearly zero-sized, yet the compiler is correct to refuse it. This is because the field might raise the alignment requirements of the transparent field, thus change the overall layout.

This patch changes the error message to distinguish non-zero-sized fields with known layout from fields with unknown layout. The logic when to emit errors is not changed. Before this patch, it would emit an error when non_zst_count >= 2 (but non_zst_count counted fields without layout AND fields with guaranteed non-zero layout). After this patch, non_zst_count is changed to only count fields with guaranteed non-zero layout, but non_layout_count counts fields without layout. The new non_transparent_count is the sum of both and should be equivalent to the previous non_zst_count.

Long story short, the new error output looks like this:

error[E0690]: transparent struct needs at most one field of non-zero size or alignment larger than 1, but has 2
 --> src/foobar.rs:2:1
  |
2 | struct Test<T: Sized> {
  | ^^^^^^^^^^^^^^^^^^^^^ needs at most one field of non-zero size or alignment larger than 1, but has 2
3 |     a: u32,
  |     ------ this field is non-zero-sized
4 |     b: [T; 0],
  |     --------- this field may have alignment larger than 1

Previously, any field without known layout was annotated as "non-zero size", now this is changed to annotate those fields as "may have alignment larger than 1". This is a catch-all that is, to my knowledge, true for all generic fields without layout, since one cannot control their alignment. This might need refinement in the future, when other cases of layout-violation occur.

One might even argue that the entire error-message could be made more generic, ala:

transparent struct needs at most one field with non-trivial layout

However, I think the current error-message is much more helpful in stating the 2 obvious violations: non-ZST and non-alignment-of-1 Hence, this commit retains the original error-message style and simply extends it with the alignment-violation notice.

Originally reported in: #98064

@rustbot
Copy link
Collaborator

rustbot commented Jul 24, 2023

r? @lcnr

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 24, 2023
@lcnr
Copy link
Contributor

lcnr commented Jul 25, 2023

r? compiler

@rustbot rustbot assigned wesleywiser and unassigned lcnr Jul 25, 2023
@apiraino
Copy link
Contributor

heh ... r? compiler

@pnkfelix
Copy link
Member

r? @pnkfelix

@rustbot rustbot assigned pnkfelix and unassigned compiler-errors Aug 24, 2023
@pnkfelix
Copy link
Member

@dvdhrm this is awesome work. Can you make the changes I have suggested to the diagnostic output, which I hope will make it both clearer and more succinct ?

@pnkfelix pnkfelix added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 24, 2023
Postpone the alignment calculation and retain the entire layout
information when checking transparent types for violations. This allows
for future diagnostic improvements because we now know the exact type
layouts of violating fields, or know exactly why their layout was not
available.
Extend the error-messages of rustc to distinguish between non-zero-sized
fields and fields without layout when annotating `repr(transparent)`
violations.

When compiling `repr(transparent)` structures, the compiler checks that
there is at most one field with non-zero size *OR* having unknown
layout. However, the error message treats both conditions as the same,
thus always annotating them as;

    a: u32,
    ------ this field is non-zero-sized

In most situations, this is reasonable. However, if you consider generic
fields, this can easily become confusing. Consider the following error
message:

    a: [T; 0],
    --------- this field is non-zero-sized

This field is clearly zero-sized, yet the compiler is correct to refuse
it. This is because the field might raise the alignment requirements of
the transparent field, thus change the overall layout.

This patch changes the error message to distinguish non-zero-sized
fields _with_ known layout from fields with _unknown_ layout. The logic
_when_ to emit errors is not changed. Before this patch, it would emit
an error when `non_zst_count >= 2` (but `non_zst_count` counted fields
without layout *AND* fields with guaranteed non-zero layout). After this
patch, `non_zst_count` is changed to only count fields with guaranteed
non-zero layout, but `non_layout_count` counts fields without layout.
The new `non_transparent_count` is the sum of both and should be
equivalent to the previous `non_zst_count`.

Long story short, the new error output looks like this:

    error[E0690]: transparent struct needs at most one field of non-zero size or alignment larger than 1, but has 2
     --> src/foobar.rs:2:1
      |
    2 | struct Test<T: Sized> {
      | ^^^^^^^^^^^^^^^^^^^^^ needs at most one field of non-zero size or alignment larger than 1, but has 2
    3 |     a: u32,
      |     ------ this field is non-zero-sized
    4 |     b: [T; 0],
      |     --------- this field may have alignment larger than 1

Previously, any field without known layout was annotated as "non-zero
size", now this is changed to annotate those fields as "may have
alignment larger than 1". This is a catch-all that is, to my knowledge,
true for all generic fields without layout, since one cannot control
their alignment. This might need refinement in the future, when other
cases of layout-violation occur.

One might even argue that the entire error-message could be made more
generic, ala:

    transparent struct needs at most one field with non-trivial layout

However, I think the current error-message is much more helpful in
stating the 2 obvious violations: non-ZST and non-alignment-of-1
Hence, this commit retains the original error-message style and simply
extends it with the alignment-violation notice.

Originally reported in: rust-lang#98064
Replace the phrase "layout of non-zero size or alignment larger than 1"
with "non-trivial layout", and then elaborate that a layout is trivial
if, and only if, it is zero-sized with an alignment of 1.

Additionally, add a comment to the check-function explaining which
information is gathered to check transparent ADTs, and elaborate why it
is needed.
@dvdhrm
Copy link
Contributor Author

dvdhrm commented Sep 4, 2023

@dvdhrm this is awesome work. Can you make the changes I have suggested to the diagnostic output, which I hope will make it both clearer and more succinct ?

Thanks! I added another commit to switch things over to the suggested wording. I did not squash it, since it seemed like a self-contained updated. But let me know if you prefer a squashed version.

@dvdhrm
Copy link
Contributor Author

dvdhrm commented Sep 15, 2023

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 15, 2023
@bors
Copy link
Contributor

bors commented Sep 17, 2023

☔ The latest upstream changes (presumably #115334) made this pull request unmergeable. Please resolve the merge conflicts.

@wesleywiser wesleywiser added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2023
@wesleywiser
Copy link
Member

Hi @dvdhrm, looks like this just needs a rebase and then we can merge it. No rush, just wanted to give you a heads up!

@pnkfelix
Copy link
Member

pnkfelix commented Oct 5, 2023

r=me once this is rebased

@dvdhrm
Copy link
Contributor Author

dvdhrm commented Oct 9, 2023

A repr(transparent) rework got filed and merged in the meantime. This PR needs a rewrite. It is not a trivial rebase.

I don't feel like spending more time on this. The constant reviewer-reassignments, waiting a month for any comment, trying to remember how rustbot works, trying to understand the changes that were merged in the meantime but couldn't even bother to write a commit message. I don't know, maybe it works for you, but as drive-by contributor, this feels very exhausting.

Maybe someone else will pick this up. Sorry.

@dvdhrm dvdhrm closed this Oct 9, 2023
@apiraino apiraino removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants