Skip to content

Deny (by default) transmuting from fn item types to pointer-sized types. #34923

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Aug 25, 2016

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jul 19, 2016

This sets the #19925 lint (transmute from zero-sized fn item type) to deny by default.
Technically a [breaking-change], but will not affect dependent crates because of --cap-lints.

@rust-highfive
Copy link
Contributor

r? @Aatch

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

@eddyb
Copy link
Member Author

eddyb commented Jul 19, 2016

@nikomatsakis This supersedes #34198, for now. I've started a three-way crater build on both PRs, to measure the impact for both changes.

@eddyb
Copy link
Member Author

eddyb commented Jul 20, 2016

Argh, the report came out half-baked. All of the cases I checked out are just timeouts, while updating the registry. cc @brson

@eddyb eddyb force-pushed the deny-fn-item-transmute branch from d04f81b to 7de83f0 Compare July 26, 2016 19:57
@eddyb
Copy link
Member Author

eddyb commented Jul 27, 2016

The good crater report shows 3 regressions, td_rlua, hlua_master (both of which seem abandoned), and zmq-rs, which needs a new release (cc @Ginurx).

I've also done a crater run for #34198, for which the report shows 3 more regressions (because --cap-lints is not in effect there): glutin_cocoa, carto and syslog-ng-common, all of which are using outdated versions of fixed crates.

@nikomatsakis
Copy link
Contributor

cc @rust-lang/compiler @rust-lang/core -- another question of making warnings into a hard error, though in this case we opted for a more gradual approach by making the lint become Deny By Default, since the impact of this change was initially large.

@eddyb's measurements show 3 regressions:

The good crater report shows 3 regressions, td_rlua, hlua_master (both of which seem abandoned), and zmq-rs, which needs a new release (cc @Ginurx).

My vote is let's go for it.

@alexcrichton
Copy link
Member

🍈

(I'm in favor)

@steveklabnik
Copy link
Member

👍

@eddyb
Copy link
Member Author

eddyb commented Jul 31, 2016

@nikomatsakis Are we go?

@nikomatsakis
Copy link
Contributor

@eddyb interestingly, @wycats was just telling me that he thinks the lint may be missing some scenarios in this code. He's going to investigate a bit.

Seems worth knowing about before we pull the trigger.

@nikomatsakis
Copy link
Contributor

@wycats any updates here? Do you have examples where you think the lint is failing to fire or be reported?

@nikomatsakis
Copy link
Contributor

OK, I think we should land this. It's not irreversible.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 25, 2016

📌 Commit 7de83f0 has been approved by nikomatsakis

bors added a commit that referenced this pull request Aug 25, 2016
Deny (by default) transmuting from fn item types to pointer-sized types.

This sets the #19925 lint (transmute from zero-sized fn item type) to `deny` by default.
Technically a `[breaking-change]`, but will not affect dependent crates because of `--cap-lints`.
@bors
Copy link
Collaborator

bors commented Aug 25, 2016

⌛ Testing commit 7de83f0 with merge 71bdeea...

@bors bors merged commit 7de83f0 into rust-lang:master Aug 25, 2016
@eddyb eddyb deleted the deny-fn-item-transmute branch August 25, 2016 21:40
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 8, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants