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

Add lint for using a type with a destructor in a zero-length repeat expr #74857

Closed

Conversation

Aaron1011
Copy link
Member

Currently, writing a zero-length array repeat expression (e.g.
[String::new(); 0]) will cause the initializer value to be leaked.
See #74836 for more details

There are three ways that we could potentially handle this case:

  1. Run the destructor immediately after 'constructing' the zero-length
    array.
  2. Run the destructor when the initializer value would otherwise be
    dropped (i.e. at the end of the lexical scope)
  3. Keep the current behavior and continue to leak to the initializer

Note that the 'obvious' choice (running the destructor when the array is
dropped) does not work, since a zero-length array does not actually
store the value we want to drop.

All of these options seem likely to be surprising and inconsistent
to users. Since any fully monomorphic constant can be used as the repeat
length, this has the potential to cause confusing 'action at a distance'
(e.g. changing a const from 0 to 1 results in drop order changing).

Regardless of which option we decide on, we should tell users
to use an empty array ([]) instead.

This commit adds a new lint ZERO_REPEAT_WITH_DROP, which fires when a
type that (potentially) has a destructor is used in a zero-length array
repeat expression.

If a type has no drop glue, we skip linting, since dropping it has no
user-visible side effects. If we do not know if a type has drop glue
(e.g. Option<T>), we lint anyway, since some choice of generic
arguments could trigger the strange drop behavior.

If the length const expression is not fully monomorphic (e.g. contains a
generic parameter), the compiler requires the type used in the repeat
expression to be Copy. Therefore, we don't need to lint in this case,
since a Copy type can never have drop glue.

Currently, writing a zero-length array repeat expression (e.g.
`[String::new(); 0]`) will cause the initializer value to be leaked.
See rust-lang#74836 for more details

There are three ways that we could potentially handle this case:

1. Run the destructor immediately after 'constructing' the zero-length
   array.
2. Run the destructor when the initializer value would otherwise be
   dropped (i.e. at the end of the lexical scope)
3. Keep the current behavior and continue to leak to the initializer

Note that the 'obvious' choice (running the destructor when the array is
dropped) does not work, since a zero-length array does not actually
store the value we want to drop.

All of these options seem likely to be surprising and inconsistent
to users. Since any fully monomorphic constant can be used as the repeat
length, this has the potential to cause confusing 'action at a distance'
(e.g. changing a `const` from 0 to 1 results in drop order changing).

Regardless of which option we decide on, we should tell users
to use an empty array (`[]`) instead.

This commit adds a new lint ZERO_REPEAT_WITH_DROP, which fires when a
type that (potentially) has a destructor is used in a zero-length array
repeat expression.

If a type has no drop glue, we skip linting, since dropping it has no
user-visible side effects. If we do not know if a type has drop glue
(e.g. `Option<T>`), we lint anyway, since some choice of generic
arguments could trigger the strange drop behavior.

If the length const expression is not fully monomorphic (e.g. contains a
generic parameter), the compiler requires the type used in the repeat
expression to be `Copy`. Therefore, we don't need to lint in this case,
since a `Copy` type can never have drop glue.
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2020
|lint| {
lint
.build("used a type with a destructor in a zero-length repeat expression")
.note(&format!("the value used here has type `{}`, which may have a destructor", ty))
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, we would use the proper spans for these notes, but it doesn't seem worth the effort to pass them through into the built MIR. We could move this to HIR typeck, but it seemed preferrable to keep this repeat-length checking logic in one place.

@SimonSapin
Copy link
Contributor

There are three ways that we could potentially handle this case:

  1. Run the destructor immediately after 'constructing' the zero-length array.

This first way seems obviously correct to me, as discussed in #74836 (comment). It’s also consistent with vec![a; 0] (thought that one requires Clone).

I’m not sure a lint is useful once the current behavior (which is a bug) is fixed.

@petrochenkov
Copy link
Contributor

A whole new lint for this looks like an overkill to me as well, it's better to run the destructor.

r? @RalfJung
(Fell free to reassign to someone else familiar with MIR.)

@petrochenkov petrochenkov assigned RalfJung and unassigned RalfJung Jul 28, 2020
@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2020

As I said in #74836, I do not think such a lint makes much sense. We don't want people to stop using zero-length arrays. We should rather fix our MIR building.

@RalfJung RalfJung 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 7, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Aug 7, 2020

I agree, we should just fix this leak. We don't guarantee running or not running destructors, so making sure it runs will not be a breaking change but a bugfix. People relying on destructors not running should use std::mem::forget

@bstrie
Copy link
Contributor

bstrie commented Nov 30, 2020

As a follow-up to this, please see #79580 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants