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

Regression in fxsm, Rust 1.17 #40966

Closed
brson opened this issue Mar 31, 2017 · 11 comments
Closed

Regression in fxsm, Rust 1.17 #40966

brson opened this issue Mar 31, 2017 · 11 comments
Assignees
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Milestone

Comments

@brson
Copy link
Contributor

brson commented Mar 31, 2017

https://github.com/vityafx/fxsm

brian@ip-10-145-43-250:~/dev/fxsm⟫ git log -1
commit 9e51313cddea3172527ca90abd2bab71cc5ad537
Author: Victor Polevoy <vityatheboss@gmail.com>
Date:   Wed Mar 1 18:30:31 2017 +0400

    Update README.md

brian@ip-10-145-43-250:/mnt2/dev⟫ rustc +beta -Vv
rustc 1.17.0-beta.1 (408d49e60 2017-03-14)
binary: rustc
commit-hash: 408d49e6087f87371e96277c5ff428fa22cb84e6
commit-date: 2017-03-14
host: x86_64-unknown-linux-gnu
release: 1.17.0-beta.1
LLVM version: 3.9

101 brian@ip-10-145-43-250:~/dev/fxsm⟫ cargo +beta test
   Compiling unicode-xid v0.0.4
   Compiling quote v0.3.14
   Compiling fxsm v0.2.0 (file:///mnt2/dev/fxsm/fxsm)
   Compiling synom v0.11.2
   Compiling syn v0.11.8
   Compiling fxsm-derive v0.2.0 (file:///mnt2/dev/fxsm/fxsm-derive)
   Compiling fxsm-test v0.1.0 (file:///mnt2/dev/fxsm)
error: proc-macro derive panicked
 --> examples/game_cup.rs:6:24
  |
6 | #[derive(Clone, Debug, StateMachine)]
  |                        ^^^^^^^^^^^^
  |
  = help: message: How were you able to call me without derive!?!?

error: Could not compile `fxsm-test`.
Build failed, waiting for other jobs to finish...
error: build failed

cc @vityafx

@brson brson added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Mar 31, 2017
@iddm
Copy link

iddm commented Mar 31, 2017

Yes, I know that. Thank you very much for highlighting me, I appreciate it! You guys do very nice work testing the beta on crates! I will fix it soon. Is there anything I should do in this issue?

@brson
Copy link
Contributor Author

brson commented Mar 31, 2017

@vityafx Nope! Just making you aware. If you determine this is our bug, please let us know the details.

@nikomatsakis
Copy link
Contributor

Glancing briefly at the code, the error arises because the code fails to find a #[derive] attribute on the enum to which it is applied. It is trying to peek and see that Copy and Clone are also derived.

cc @jseyfried -- do you know of any changes we made in this area, i.e., in terms of what tokens we supply to derive implementations?

@vityafx -- another way to enforce this, though UI will suffer, would be to generate code that requires Copy and Clone, at worst by artificially calling something like

fn require_copy_and_clone<T: Copy+Clone>(t: &T) { }

@iddm
Copy link

iddm commented Apr 4, 2017

@nikomatsakis yes, I know that too. There was a change which makes impossible to see what does the enum/struct derive but fxsm relies on this for determination of what code to produce - one which uses Copy or one which uses Clone. I have totally forgot about what Copy and Clone are, so after some discussion in rust issues page, I have been told to provide only implementation for a Clone because Copy is just a marker. So this may simply be rewritten correctly. I just have not been able to find the time for this yet unfortunately.

@arielb1 arielb1 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 4, 2017
@jseyfried
Copy link
Contributor

This was intentionally caused by #39572. See #40574 (comment) and #40574 (comment) for rationale; #39572 (comment) is an alternative.

cc @rust-lang/lang

@jseyfried jseyfried self-assigned this Apr 5, 2017
@arielb1 arielb1 added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Apr 5, 2017
@nikomatsakis
Copy link
Contributor

@jseyfried interesting; I'll have to catch up in more depth. It seems like it is common to want to "peek" at what other derives are present, so that you can generate somewhat better/different derives for your own trait. It's not perfect, though, since of course there could be manual impls...

@nikomatsakis
Copy link
Contributor

@vityafx ok I agree that the current thing in your particular case is to generate code that uses foo.clone().

@pnkfelix
Copy link
Member

pnkfelix commented Apr 6, 2017

discussed at compiler meeting. Consensus is that if there is a bug here, it is in fact a feature request for the procedural macro system (the feature being having a way to peek at the other derives), and that would be the balliwick of the lang team to address.

So, removing T-compiler label. (And also suggesting that the lang team close after reviewing it.)

@pnkfelix pnkfelix removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 6, 2017
@alexcrichton alexcrichton added this to the 1.17 milestone Apr 10, 2017
@nikomatsakis
Copy link
Contributor

Nominated for @rust-lang/lang team discussion.

@alexcrichton
Copy link
Member

The lang team is likely to skip their meeting today so this unfortunately won't get a chance for discussion before 1.17 is release, which means that this is likely to become a stable-to-stable regression.

@nikomatsakis was thinking though that the most likely outcome is to uphold the compiler team's decision of staying the current course, though.

@vityafx out of curiosity do you need any help updating your crate? I can try to help out if necessary!

@alexcrichton alexcrichton added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Apr 26, 2017
@brson
Copy link
Contributor Author

brson commented May 4, 2017

@rust-lang/lang says this is a bugfix.

@brson brson closed this as completed May 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants