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 warning for inner function marked as `#[test]` #2471

Merged
merged 2 commits into from Sep 2, 2018

Conversation

Projects
None yet
10 participants
@estebank
Contributor

estebank commented Jun 10, 2018

Rendered
Tracking issue

Add a lint that warns when marking an inner function as #[test].

#[test] is used to mark functions to be run as part of a test suite. The
functions being marked need to be addressable for this to work. Currently,
marking an inner function as #[test] will not raise any errors or warnings,
but the test will silently not be run. By adding a lint that identifies these
cases, users are less likely to fail to notice.

Implemented in rust-lang/rust#51450.

@warlord500

This comment has been minimized.

warlord500 commented Jun 11, 2018

I have a quick question, why do inner test functions not run?

a good use case for inner test function is the outer function you have a const you dont want to expose to the world. but in the inner function, you need test the outer function with the const as input for example something like sin function with pi as input but obviously the const is supposed to be hidden.

@estebank

This comment has been minimized.

Contributor

estebank commented Jun 11, 2018

@warlord500 they cannot run because test functions need to be addressable, and inner functions aren't.

cc rust-lang/rust#36629

@Centril Centril added the T-lang label Jun 11, 2018

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Jun 12, 2018

Does this need an RFC? It sounds like a bug fix to me.

@estebank

This comment has been minimized.

Contributor

estebank commented Jun 12, 2018

@Centril Centril added the I-nominated label Jun 12, 2018

@sfackler

This comment has been minimized.

Member

sfackler commented Jun 12, 2018

@eddyb

This comment has been minimized.

Member

eddyb commented Jun 12, 2018

What if we use hygiene to force name resolution of the harness paths (well, they can be just single identifiers then) to occur in the definition scope of the test functions?
It could even make the harness neatly pluggable - I mean, is it different than injecting a single macro invocation with all of those hygiene-tagged names, at the top-level of the crate?

cc @petrochenkov @alexcrichton @Manishearth

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Jun 13, 2018

Looks like for each fn marked with #[test] we need to

  • create a fresh "macro 2.0" macro
  • register that macro as coming from the same location as fn
  • mark all fn's tokens as produced by that macro
  • copypaste the marked tokens into whatever place the test harness needs

Sounds possible if expansion of #[test] has access to full compiler APIs and runs at correct phase, not sure about details.

@eddyb

This comment has been minimized.

Member

eddyb commented Jun 14, 2018

@petrochenkov That's an interesting approach, but wouldn't it suffice to only have a single ExprPath that resolves to a specific function without being anywhere near it?

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Jun 14, 2018

Ah, I see, then it should look something like:

  • create a fresh "macro 2.0" macro
  • register that macro as coming from the same location as fn
  • mark the ExprPath as produced by that macro
  • copypaste the ExprPath into whatever place the test harness needs
@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Jun 14, 2018

However, if we can do what I wrote above, then we can probably directly assign resolution to the ExprPath's node id as well.

@eddyb

This comment has been minimized.

Member

eddyb commented Jun 14, 2018

@petrochenkov Yeah, the main reason I wanted to avoid dealing with resolution directly is for moving the test harness into a library macro, so it would literally only get one ident token per fn.

@estebank

This comment has been minimized.

Contributor

estebank commented Jun 14, 2018

We would still meet to make sure there are no rest functions nested inside another test function, right?

@eddyb

This comment has been minimized.

Member

eddyb commented Jun 14, 2018

@estebank I don't see how that would be a problem.

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Jun 14, 2018

We discussed this in the lang team meeting. Our conclusion: because this doesn't work today, we should have a lint about it. If someone wants to propose a separate RFC that defines a behavior for this, they can also propose a change to the lint.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

rfcbot commented Jun 14, 2018

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Jun 14, 2018

As an aside, perhaps things like putting #[test] on a trait method should emit the same lint?

@scottmcm

This comment has been minimized.

Member

scottmcm commented Jun 14, 2018

One possible way it wouldn't need an RFC might be if it's a fix-a-false-negative extension of the existing unused_attributes lint, as that's already emitted by

trait Foo {
    #[test]
    fn foo() {}
}
warning: unused attribute
 --> src/main.rs:2:5
  |
2 |     #[test]
  |     ^^^^^^^
  |
  = note: #[warn(unused_attributes)] on by default
@eddyb

This comment has been minimized.

Member

eddyb commented Jun 14, 2018

I agree with @scottmcm, if we can make the unused_attribute lint fire, organically.
Otherwise, I strongly prefer a proper solution, along the lines of what @petrochenkov described.

@scottmcm

This comment has been minimized.

Member

scottmcm commented Aug 20, 2018

Per rust-lang/rust#36629 (comment), this was done without the RFC being accepted?

@Centril

This comment has been minimized.

Contributor

Centril commented Aug 20, 2018

(This is currently not implemented on stable but it is implemented in the beta compiler, which becomes stable on 2018-09-13)

@estebank

This comment has been minimized.

Contributor

estebank commented Aug 21, 2018

It can safely be reverted. I forgot about the RFCS I'd opened when I pinged on the PR and it got approved.

@scottmcm

This comment has been minimized.

Member

scottmcm commented Aug 23, 2018

I still think this should be unused_attribute, but linting it is what's really important.

@rfcbot reviewed

@eddyb

This comment has been minimized.

Member

eddyb commented Aug 23, 2018

Hmm, does unused_attributes not fire because of the #[cfg(test)] gating, or because of the fact that the nested functions are still items and some part of the traversal hits them?

For either, I think we can move the bit that looks at #[test] attributes to look only at item children of modules, which is the only place where #[test] fn works.
I could look into it or mentor someone through it.

@rfcbot

This comment has been minimized.

rfcbot commented Aug 23, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

rfcbot commented Sep 2, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@Centril Centril merged commit 26a847f into rust-lang:master Sep 2, 2018

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 2, 2018

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#53911

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