Skip to content

Add a lint for non-exported local types in visible type signatures. #11773

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

Closed
wants to merge 3 commits into from

Conversation

huonw
Copy link
Member

@huonw huonw commented Jan 24, 2014

Except for traits, to allow them to be used as closed type-classes and/or PIMPL-style polymorphic trait objects

struct Private;
pub fn warning(x: Private) {}

trait Priv {}
pub fn ok<T: Priv>() {}

huonw added 2 commits January 24, 2014 23:41
The fundamental bug (expressions in fixed-length vectors make the lint
passes unhappy) is still there, so we just sneak around it by (manually)
avoiding recurring into that expression.
This warns whenever a publically visible type (e.g. in the signature of
a public function, or the non-priv field of a public struct) is not
exported.
@huonw
Copy link
Member Author

huonw commented Jan 24, 2014

This ends up encoding some logic about whether a type is local or not inside lint, which makes it less nice... (If this doesn't land, I'm going to submit (most of) the last patch separately because there's a pile of accidentally non-exported types (or non-priv fields).)

@alexcrichton
Copy link
Member

In general, I like the idea for this lint, but I'd like to garner some more opinions on this (I don't think we've quite reached a consensus on #10573 yet).

Implementation-wise, I'm troubled to see how invasive it is to the other lint passes and the visiting context itself. I would expect this to be a pretty self contained lint which only checks function signatures, structs, and enums (checks in visit_item and visit_fn). It looks like the invasive parts elsewhere are mainly to get a better error message, which I think is ok to sacrifice for complexity reduction, but I may have also missed a point where they're more crucial to the analysis.

@huonw
Copy link
Member Author

huonw commented Jan 25, 2014

I'm also unhappy how invasive this is, but, unfortunately, I don't see any other way to handle it, other than doing some manual visit_type calls in visit_fn and visit_item. I guess I can try it.

Re #10573: I've implemented this because I think it's very bad code style to have private types in public code signatures (e.g. users of your library can no longer name the type that functions return; e.g. you can't write wrappers around them, you can only use call the functions directly) and so I'd like to have the option for the compiler to warn about it whether or not we decide to outright disallow it.

@alexcrichton
Copy link
Member

I talked with @huonw on IRC about this, and he's going to look into either simplifying the logic to have it a little more contained, or move the logic to its own pass in middle::privacy.

@huonw
Copy link
Member Author

huonw commented Jan 30, 2014

This is on the backburner for now; I'll get back to it after I'm done with #[deriving(Show)].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants