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

Don't use underscores in argument names for derive output #49676

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@Manishearth
Member

Manishearth commented Apr 5, 2018

Currently in the docs #[derive()]d functions show up with arguments like __arg0, __arg1, etc.

Ideally they'd have descriptive names set by the derive, but for now I'd just like to remove the underscores.

The underscores exist so that the unused_variable lint isn't triggered, but there's a simpler solution there -- don't run that pass for these functions.

In a followup I'll make it possible for consumers of the builtin derive helpers to specify argument names; or teach rust/rustdoc to scrape them off the trait.

r? @jseyfried @eddyb

cc @QuietMisdreavus

@TimNN

This comment has been minimized.

Contributor

TimNN commented Apr 5, 2018

Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:00:47] configure: rust.quiet-tests     := True
---
[00:20:20] error: unused variable: `arg_0`
[00:20:20]    --> libcore/fmt/mod.rs:104:43
[00:20:20]     |
[00:20:20] 104 | #[derive(Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
[00:20:20]     |                                           ^^^^ help: consider using `_arg_0` instead
---
[00:20:20]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name core libcore/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=2 -C metadata=64878136ec7adadb -C extra-filename=-64878136ec7adadb --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/release/deps` (exit code: 101)
[00:20:20] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:20:20] expected success, got: exit code: 101
[00:20:20] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1075:9
[00:20:20] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:20:20] travis_fold:end:stage1-std
[00:20:20] travis_time:end:stage1-std:start=1522892524791158261,finish=1522892560294824940,duration=35503666679
[00:20:20] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:20:20] Build completed unsuccessfully in 0:15:40
[00:20:20] make: *** [all] Error 1
[00:20:20] Makefile:28: recipe for target 'all' failed

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Apr 5, 2018

Also fixed this hygiene issue

@TimNN

This comment has been minimized.

Contributor

TimNN commented Apr 5, 2018

Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:00:45] configure: rust.quiet-tests     := True
---
[00:20:29] error: unused variable: `arg_0`
[00:20:29]   --> libcore/ops/range.rs:48:38
[00:20:29]    |
[00:20:29] 48 | #[derive(Copy, Clone, PartialEq, Eq, Hash)]
[00:20:29]    |                                      ^^^^ help: consider using `_arg_0` instead
---
[00:20:29]    = note: #[deny(unused_variables)] implied by #[deny(warnings)]
[00:20:29]
[00:20:29] error: unused variable: `arg_0`
[00:20:29]     --> libcore/option.rs:1219:62
[00:20:29]      |
[00:20:29] 1219 | #[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)]
[00:20:29]      |                                                              ^^^^ help: consider using `_arg_0` insteaderror: aborting due to 3 previous errors
[00:20:29]
[00:20:29] error: Could not compile `core`.
[00:20:29]
[00:20:29] Caused by:
[00:20:29]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name core libcore/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=2 -C metadata=64878136ec7adadb -C extra-filename=-64878136ec7adadb --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/release/deps` (exit code: 101)
[00:20:29] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:20:29] expected success, got: exit code: 101
[00:20:29] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1075:9
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:0080f462:start=1522894067532624521,finish=1522894067540015082,duration=7390561
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:172d5d91
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN.

@TimNN

This comment has been minimized.

Contributor

TimNN commented Apr 5, 2018

Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:00:48] configure: rust.quiet-tests     := True
---
[00:07:08] error[E0412]: cannot find type `Item` in this scope
[00:07:08]    --> librustc/middle/liveness.rs:188:39
[00:07:08]     |
[00:07:08] 188 |     fn visit_item(&mut self, i: &'tcx Item) {
[00:07:08]     |                                       ^^^^ not found in this scope
[00:07:08] help: possible candidates are found in other modules, you can import them into scope
---
[00:07:42] Makefile:28: recipe for target 'all' failed
[00:07:42] make: *** [all] Error 1

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN.

@TimNN

This comment has been minimized.

Contributor

TimNN commented Apr 5, 2018

Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:00:45] configure: rust.quiet-tests     := True
---
[00:26:46] error: unused variable: `arg_0`
[00:26:46]   --> libcore/ops/range.rs:48:38
[00:26:46]    |
[00:26:46] 48 | #[derive(Copy, Clone, PartialEq, Eq, Hash)]
[00:26:46]    |                                      ^^^^ help: consider using `_arg_0` instead
---
[00:26:46]    = note: #[deny(unused_variables)] implied by #[deny(warnings)]
[00:26:46]
[00:26:46] error: unused variable: `arg_0`
[00:26:46]     --> libcore/option.rs:1219:62
[00:26:46]      |
[00:26:46] 1219 | #[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)]
[00:26:46]      |                                                              ^^^^ help: consider using `_arg_0` instead
[00:26:46]
[00:26:46] error: unused variable: `arg_0`
[00:26:46]    --> libcore/fmt/mod.rs:104:43
[00:26:46]     |
[00:26:46] 104 | #[derive(Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
[00:26:46]     |                                           ^^^^ help: consider using `_arg_0` instead
---
[00:26:46]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name core libcore/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=2 -C metadata=64878136ec7adadb -C extra-filename=-64878136ec7adadb --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/release/deps` (exit code: 101)
[00:26:46] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:26:46] expected success, got: exit code: 101
[00:26:46] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1075:9
[00:26:46] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:26:46] travis_fold:end:stage1-std
[00:26:46] travis_time:end:stage1-std:start=1522896251938877462,finish=1522896304702149128,duration=52763271666
[00:26:46] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:26:46] Build completed unsuccessfully in 0:21:03
[00:26:46] make: *** [all] Error 1
[00:26:46] Makefile:28: recipe for target 'all' failed
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:0b4036c2:start=1522896305293935512,finish=1522896305302693988,duration=8758476
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:0ad5f5a6
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:0ad5f5a6:start=1522896305310069248,finish=1522896305317522163,duration=7452915
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:09350d47
$ dmesg | grep -i kill
[   10.683938] init: failsafe main process (1093) killed by TERM signal

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Apr 5, 2018

Argh I need a clean way to do this.

I guess I can just check the span but that's kinda cheating :/

@Manishearth

This comment has been minimized.

Member

Manishearth commented Apr 5, 2018

Oh, that's an older failure.

@TimNN looks like the bot shows failures from travis builds on stale commits?

(Also, would be nice to have a way to ask the bot to not post for WIP PRs, either by detecting WIP in the title or a @timnnbot shh command)

@Manishearth

This comment has been minimized.

Member

Manishearth commented Apr 5, 2018

Stuff passes now, but the partialord stuff isn't fixed:

error[E0530]: match bindings cannot shadow constants
 --> test.rs:9:7
  |
4 | pub const cmp: u8 = 1;
  | ---------------------- a constant `cmp` is defined here
...
9 |     B(u8)
  |       ^^^ cannot be named the same as a constant

because cmp @ _ is not enough to avoid shadowing.

Which makes me realize that the __self_0 stuff is broken too, because:

pub const __self_0: u8 = 1;

#[derive(PartialOrd, PartialEq)]
pub enum Foo {
    A(u8), B(u8)
}

will hit it on the ref bindings.

Yay.

@TimNN

This comment has been minimized.

Contributor

TimNN commented Apr 5, 2018

@Manishearth: Thanks for the feedback! Both features are already planned and should be implemented by the end of the week(end).

@Manishearth

This comment has been minimized.

Member

Manishearth commented Apr 5, 2018

That error exists because of #33118, specifically because

pub const A = ...
let A = ...

match ... {
  A => ...
}

will resolve A in the match as a binding, however this will not occur if the let doesn't exist. Disallowing shadowing constants entirely is the duct tape to keep this from being weird. Hmm.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Apr 5, 2018

@TimNN thanks!

@Manishearth

This comment has been minimized.

Member

Manishearth commented Apr 5, 2018

At least for __cmp we still have a fix: We can do it as a two step pass where we do let foo = cmp(..); match cmp { Equal => ..., _ => cmp}. However the __self stuff is used inside proper pattern matches which we need to make work somehow. So the cmp stuff can't be fixed.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Apr 5, 2018

(Since I removed the commit now, my above rambling is about a potential fix I had for #49679 )

@Manishearth

This comment has been minimized.

Member

Manishearth commented Apr 5, 2018

Ugh, we still have other issues:

pub const arg_0: u8 = 1;
fn foo(arg_0: bool) {}

itself will type error because arg_0 the pattern resolves to the constant, not a binding. So keeping the underscores may be for the best since it's harder to have a shadowing constant that way.

The ability to use constants as patterns should really be removed for patterns in an irrefutable context. It is already disallowed, but not at the right stage.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Apr 5, 2018

Blocked on #49680

@eddyb

This comment has been minimized.

Member

eddyb commented Apr 5, 2018

Note the difference between unhygienic old macros and hygienic new macros.

if let FnKind::Method(..) = fk {
let parent = ir.tcx.hir.get_parent(id);
if let Some(hir::map::Node::NodeItem(i)) = ir.tcx.hir.find(parent) {
if i.attrs.iter().any(|a| a.check_name("automatically_derived")) {

This comment has been minimized.

@durka

durka Apr 5, 2018

Contributor

Does this cover custom derive?

@pietroalbini

This comment has been minimized.

Member

pietroalbini commented Apr 9, 2018

As per #49676 (comment), this is blocked on #49680.

@eddyb

This comment has been minimized.

Member

eddyb commented Apr 10, 2018

@pietroalbini Not necessarily, see #49676 (comment).

@pietroalbini

This comment has been minimized.

Member

pietroalbini commented Apr 10, 2018

Oh, sorry, I misunderstood that comment :)

@pietroalbini

This comment has been minimized.

Member

pietroalbini commented Apr 16, 2018

Ping from triage! What's the status of this PR?

@Manishearth

This comment has been minimized.

Member

Manishearth commented Apr 16, 2018

Probably superseded by #49986, if not, I'll reopen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment