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

RFC: remove pub use globs #11870

Closed
pnkfelix opened this issue Jan 28, 2014 · 22 comments
Closed

RFC: remove pub use globs #11870

pnkfelix opened this issue Jan 28, 2014 · 22 comments
Assignees
Labels
A-resolve Area: Path resolution

Comments

@pnkfelix
Copy link
Member

A recent chat I had with pcwalton led me to believe that many of the difficulties we have with globs in use declarations would be resolved if we simply restricted globs solely to imports, i.e. removed glob re-exports via pub use.

Removing pub use globs would imply that when rustc looks at the text of a module, it can locally determine all of the names that it exports. I think this would enable us to make a much simpler resolve algorithm: one pass to associate each module with the set of names it exports, and then a second pass to wire up the glob imports. (It still doesn't necessarily know which of the type/value namespaces the imported/exported names belong to, so resolve still remains non-trivial, but I think that's okay.)

This is meant as an alternative more-conservative option to #11825. In particular, Servo's use-case as documented by jack should continue to function under this plan, if I'm correct in inferring that the auto-generated code-bindings are solely creating imports.

(globs are feature-guarded for 1.0, so choosing a strategy here need not block the release. But it would probably make people more comfortable knowing how drastic a change to globs we might make.)

(I have been careful in my weasel wording above not to claim that all problems with globs stem from pub use alone, since bugs like #3352 do not even use pub use. But I suspect such bugs are artifacts of an complicated implementation strategy necessitated by pub use, and so I still think this route is worth considering.)


I will admit up front: glob imports are not a must-have feature for me; I'd be okay if #11825 landed instead of this approach. But I do think there is value in providing a way for people who want to play with some library to snag all of the functionality it provides trivially, especially for humans who learn via play.

@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 2, 2014

@metajack can you confirm (or counter) my hypothesis above that removing pub use would be compatible with Servo's use case?

@jdm
Copy link
Contributor

jdm commented Feb 2, 2014

Servo actually has at least two uses of pub use globs: http://mxr.mozilla.org/servo/source/src/components/script/script.rc#45

@metajack
Copy link
Contributor

metajack commented Feb 2, 2014

@jdm Could we remove those easily?

@jdm
Copy link
Contributor

jdm commented Feb 2, 2014

It would certainly be unpleasant, and I may even have added it to avoid some sort of circular dependency. I don't quite recall now.

@pnkfelix
Copy link
Member Author

I would have assumed that given the option of removing all uses of globs, versus removing solely pub use globs, that the latter would be the far more attractive option in most projects. But perhaps I am mistaken.

(edit: I speak solely in terms of pain of doing the transformation; I am not speaking on the quality of the code after the transformation, since one can reasonably claim that a codebase with no globs of any kind is easier to work with. Not that I am claiming that... :) )

@nikomatsakis
Copy link
Contributor

+1

@flaper87
Copy link
Contributor

FWIW, I like this proposal.

@brson
Copy link
Contributor

brson commented Feb 25, 2014

+1.

After removing globs from the code base there are a few use cases left where they are nice, particularly prelude injection. We need an answer for how the prelude works for 1.0.

Nominating.

@Valloric
Copy link
Contributor

This sounds like a very reasonable proposal. I'm against glob imports for production code, but for tests they're great (e.g. pulling in all the names of the module you are testing). Re-exporting everything from a different module sounds like a recipe for disaster on the user-facing side even if it worked correctly in the compiler.

+1 for preventing people from shooting themselves in the foot.

@emberian
Copy link
Member

gl-rs uses these, but it's automatically generated, we can make long lists of reexports with ease.

@pnkfelix
Copy link
Member Author

Globs are already feature-gated, but there is an important use-case for them (or at least non pub-use globs) in the prelude.

P-backcompat-lang, 1.0. (Borderline, could be P-high.)

@brson
Copy link
Contributor

brson commented Apr 8, 2014

This looks like something relatively easy that I can pick off as a secondary project. Assigning to myself. If I don't get it done and someone else wants it that's fine.

@brson brson self-assigned this Apr 8, 2014
@brson
Copy link
Contributor

brson commented Apr 8, 2014

The big problem here is probably going to be fixing liblibc which reexports everything with globs.

@pnkfelix
Copy link
Member Author

pnkfelix commented Apr 9, 2014

i wonder if a set of local macros could solve the problem for libc. (I.e. a macro that takes in some syntax representing the module tree within libc and spits out both the modules and the pub uses)

brson added a commit to brson/rust that referenced this issue Apr 9, 2014
@brson
Copy link
Contributor

brson commented Apr 9, 2014

I feel like liblibc has a lot of other problems and needs to be refactored to be more modern. At the least the stuff that it defines that is not libc should be put elsewhere.

@SiegeLord
Copy link
Contributor

I find this a bit distressing, as every single library I have written uses them extensively.

The overall summary of my use case is that while I like modules to compartmentalize and encapsulate my library code, many if not most of the module divisions are merely implementation details that I don't care to pay heed to as a user of those libraries: they just don't have any semantic usefulness. Lastly, on many occasions, the exact type names are generated by a macro which makes it quite impossible to even write a non-glob re-export without manual copy-pasting.

I would find losing the glob imports to be a much easier change to adapt to, as it merely requires prefixing everything that's not a trait (traits are a pain to have to import one-by-one if you have many fine-grained traits, but that's a general problem that glob imports merely paper over). Luckily, I don't auto-generate traits just yet.

@nikomatsakis
Copy link
Contributor

I would rather just make them work (for some def'n of work). I have a cautious plan. When I've vetted it with a few others, I'll unveil it publicly. :)

brson added a commit to brson/rust that referenced this issue Apr 15, 2014
bors added a commit that referenced this issue Apr 16, 2014
Them removes all the glob reexports from liblibc. I did it by removing them all, and then adding back per-platform explicit reexports until everything built again.

I realize this isn't the best strategy for determining an API, but this is the lowest-impact change that solves the problem, plus I'm dissatisfied with the design of this library for other reasons and think it needs to be reconsidered from top to bottom (later).

Progress on #11870.
@brson
Copy link
Contributor

brson commented Apr 17, 2014

The quasiquoter reexports the entire AST, and changing the quasiquoter is a PITA.

bors added a commit that referenced this issue Jun 2, 2014
This patchset removes `pub use` usage except for `test/`.
cc #11870
@brson
Copy link
Contributor

brson commented Jun 9, 2014

Nominating for removal from milestone. Are we going to leave globs in?

@pnkfelix
Copy link
Member Author

The local resolve experts on the team believe there exists a sound + coherent algorithm for resolve that supports both globs and pub use globs correctly.

Under the assumption that they are correct, we anticipate a future rewrite of resolve to add such support, but that rewrite may potentially happen post 1.0. (This is okay because all globs are feature gated anyway.)

Removing from 1.0 milestone.

@pnkfelix pnkfelix removed this from the 1.0 milestone Jun 12, 2014
@aturon
Copy link
Member

aturon commented Sep 18, 2014

Nominating for removal of "P-backcompat-lang". Globs are feature-gated.

@pnkfelix
Copy link
Member Author

Closing; if we do unfeature-gate globs in the future, then this should maybe be reconsidered as an idea, but chances are that such unfeature-gating would be coupled with a complete revisit of the resolve algorithm anyway.

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 1, 2023
…e-path, r=Jarcho

Use absolute path for `declare_tool_lint` in `declare_clippy_lint`

A minor change to hide that implementation detail

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-resolve Area: Path resolution
Projects
None yet
Development

No branches or pull requests

10 participants