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

recommend explicit using Foo: Foo, ... in package code (was: "using considered harmful") #42080

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

KristofferC
Copy link
Sponsor Member

@KristofferC KristofferC commented Sep 1, 2021

I feel we are heading up against a "using crisis" where any new feature that is implemented by exporting a new name (either in Base or a package) becomes a breaking change. This is already happening (JuliaGPU/CUDA.jl#1097, JuliaWeb/HTTP.jl#745) and as projects get bigger and more names are exported, the likelihood of this rapidly increases.

The flaw in using Foo is fundamental in that you cannot lexically see where a name comes from so when two packages export the same name, you are screwed. Any code that relies on using Foo and then using an exported name from Foo is vulnerable to another dependency exporting the same name.
Therefore, I think we should start to strongly discourage the use of using Foo and only recommend using Foo for ephemeral work (e.g. REPL work).

@KristofferC KristofferC added the domain:docs This change adds or pertains to documentation label Sep 1, 2021
Copy link
Member

@DilumAluthge DilumAluthge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also say that you can just do import Foo instead of using Foo: Foo?

@DilumAluthge DilumAluthge added backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Sep 1, 2021
base/docs/basedocs.jl Outdated Show resolved Hide resolved
@mbauman
Copy link
Sponsor Member

mbauman commented Sep 1, 2021

What's the status of tooling support here? I'd say that's required before we make any such proclamation.

@ararslan
Copy link
Member

ararslan commented Sep 1, 2021

Good point about tooling. Something very nice that Racket's IDE DrRacket does is that it shows you the source of a binding when you hover over it. That very nicely mitigates any confusion when reading code over the source of a particular binding.

Also note that my suggested change to the verbiage makes this more informative and less prescriptive, which could be worthwhile regardless.

@DilumAluthge
Copy link
Member

DilumAluthge commented Sep 1, 2021

less prescriptive

Personally, I think we should be more prescriptive. As Kristoffer points out, this kind of use of using is actively breaking packages.

@ararslan
Copy link
Member

ararslan commented Sep 1, 2021

I think it should be left up to code authors how they want to write their code. We can provide information that may inform their decision, but saying "Julia provides a way for you to do this but don't do it lul" isn't helpful. People find and fix breakages on the (in my experience, rare) occasions that they occur, users can qualify bindings or do explicit loads if they choose.

When Base adds a new export, we should probably run PkgEval to see what effect it has. It would also be nice if packages with many dependants had reverse testing on new tags. That way package authors can be better equipped to work together to provide a good experience for users.

@tkf
Copy link
Member

tkf commented Sep 1, 2021

People (including me) don't read documentation carefully always. Starting with a concise recommendation followed by reasoning seems to be the best practical strategy. If they disagree with the recommendation and understand the reasoning, it is of course "left up to code authors how they want to write their code."

One missing reasoning step in the current version is that, adding features 1 is not considered breaking and can be done without incrementing the major version according to semver. So, if you only bound the major version and not minor version, you simply cannot 2 use using Foo; (strictly speaking). This gives Julia programmers another strategy: they can use using Foo; and bound the minor version of all dependencies.

Footnotes

  1. I'm implicitly assuming exporting a new name is considered a feature addition. I believe this is the common understanding but I don't know if that's explicitly noted.

  2. One exception is using Foo; in a baremodule which is still semver-compatible.

@jlapeyre
Copy link
Contributor

jlapeyre commented Sep 2, 2021

In many situations, I favor using Foo: Foo or import Foo and then Foo.a. If a is a long identifier that is heavily used, then maybe using Foo: long_identifier. You might want to mention it as an option for solving the problem. There are arguments for and against fully qualifying names that I won't go into here.

In any case, I haven't seen a good defense for using Foo in library code.

The admonition seems like it belongs in some kind of style guide. And tooling might include an option in linters, with an option at some level to error on warnings. I don't see why it needs a certain level of tooling support before the recommendation is published. But, without tooling, I suspect adoption will remain weak. I'm thinking of the ways PEPs and pylint work in Python projects and clippy in rust. I not suggesting the big administrative burden of the whole PEP system, just that starting to adopt styles and tooling to handle complexity as the amount of code increases is wise.

@domluna
Copy link

domluna commented Sep 2, 2021

It could be an option for JuliaFormatter, there was talk about this in a slack thread. The thing about that is I think the only people who would turn it on are people who already know about the pitfalls using using in such a manner. So it would be preaching to the choir in a sense.

@tkf
Copy link
Member

tkf commented Sep 2, 2021

Arguably, there's nothing new in this PR. It has been (more or less) possible to derive this from the semver specification.

While discussion on tooling is important on its own, I believe a more fundamental aspect is the interaction of semver and Julia's package/module system. Making it crystal clear even for those who haven't read the semver specification will help to reduce "controversies" of choosing the behavior of such tools. (I'm sorry for RTFM'ing. But I feel that this is an important point here.)

@johnnychen94
Copy link
Sponsor Member

johnnychen94 commented Sep 2, 2021

I would instead recommend the other side: be conservative on what you should export and think about the naming ambiguity.

For instance, exporting a width trait from any package should be discouraged IMHO because 1) this trait is mostly for developers and not for users or convenient usage 2) this name is so common that nobody except for Base should declare the ownership on it.

From a maintenance perspective, exporting a function at will also makes it very difficult to deprecate and remove the symbol.

@timholy
Copy link
Sponsor Member

timholy commented Sep 2, 2021

I think this is perhaps stronger advice than required. How about pointing out the problem and proposing a way to address it without prescribing that "you shouldn't do this." Personally I'm OK with a rare breakage if I get to write nicer code internally in my packages.

@KristofferC
Copy link
Sponsor Member Author

What's the status of tooling support here? I'd say that's required before we make any such proclamation.

Out of curiosity, why? The presence or absence of tooling doesn't change any of the facts.
Sure, having tooling to convert a using Foo into explicit names would make it easier to move away from naked using but I don't see how it changes anything concrete.

@KristofferC
Copy link
Sponsor Member Author

I think it should be left up to code authors how they want to write their code. We can provide information that may inform their decision, but saying "Julia provides a way for you to do this but don't do it lul" isn't helpful.

Sure, this is why this is only a recommendation and this section just states the recommendation and the reason for it (exporting new names becomes a breaking change).

@timholy
Copy link
Sponsor Member

timholy commented Sep 2, 2021

But the language is still quite prescriptive: "X is not recommended, do Y instead" vs "X can be messed up by Z, you can prevent this by doing Y". Subtle difference, but one that I think affects perception.

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Sep 2, 2021

Personally I'm OK with a rare breakage if I get to write nicer code internally in my packages.

Just to give you an example of what this leads to (JuliaWeb/HTTP.jl#745 (review)). HTTP.jl now does type piracy on closewrite(::Any) because they consider it breaking to not allow using HTTP; ...; closewrite(stream) to work. So it isn't really a "rare breakage" that gets fixed and everyone is happy but long-lasting bad situations from these choices. And while you might fix the breakage on the latest version, do you also backport that to all your other semver incompatible versions? Otherwise, people who do not want to upgrade to a new breaking version of your package are stuck with their old version not working anymore.

And with "nicer" code you mean to avoid the annoyance of having to spell out where your names come from. While you might be smart enough to remember exactly where all the names come from, I am not, so trying to look up where certain names come from is one of the main annoyances I have while reading code. Explicit mentioning where names come from would be a nice help to future contributors who don't have the namespace of all the dependencies in their head.

@tkf
Copy link
Member

tkf commented Sep 2, 2021

@ararslan @mbauman @timholy If this PR is rephrased to a mere weak recommendation, shouldn't the use of semver in packages become a weak recommendation, for consistency? See my above comments for the context.

(It's not a rhetorical question. It sounds like an option.)

@KristofferC KristofferC removed backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Sep 2, 2021
@KristofferC
Copy link
Sponsor Member Author

One missing reasoning step in the current version is that, adding features [^1] is not considered breaking and can be done without incrementing the major version according to semver. So, if you only bound the major version and not minor version, you simply cannot [^2] use using Foo; (strictly speaking).

Yes, the "axiom" here is that assuming everyone following semver and declares compatibility correctly, then we want to encourage coding in a way that keeps the code after a package update. That's why we discourage the use of type piracy, we discourage using internals of packages/Base. And as you say, an assumption is that adding a new export is not considered a breaking change (which I think is quite uncontroversial, looking at how Base does it). The conclusion from that is that since using Foo is incompatible with the two previous points, it should be discouraged.

@DilumAluthge
Copy link
Member

Any particular reason for not backporting this? I imagine that people using the LTS will be reading the 1.6 docs, and it would be good for them to see this recommendation.

@DilumAluthge
Copy link
Member

What's the status of tooling support here? I'd say that's required before we make any such proclamation.

Out of curiosity, why? The presence or absence of tooling doesn't change any of the facts.
Sure, having tooling to convert a using Foo into explicit names would make it easier to move away from naked using but I don't see how it changes anything concrete.

I agree that having tooling is not a pre-requisite for merging this PR.

@mbauman
Copy link
Sponsor Member

mbauman commented Sep 2, 2021

I'd be okay with a descriptive warning without there being tooling support.

I don't like having a prescriptive admonition without tooling support. With tooling support, I'm 100% for it (hence my conflicting thumbs). Perhaps I'm in the minority, but I don't think this is a minor point. To make a (poor) analogy, some new governmental regulations are accompanied by tax credits/grants/assistance to offset their implementation cost. We should make it easy for folks to address. Without that, it's just words that core devs will point at to blame packages for breaking.

@mbauman
Copy link
Sponsor Member

mbauman commented Sep 2, 2021

Similarly, I think a descriptive warning (like Alex's suggestion) could be back ported to 1.6/1.7, but a prescriptive one should not be.

@tkf
Copy link
Member

tkf commented Sep 2, 2021

To make a (poor) analogy, some new governmental regulations are accompanied by tax credits/grants/assistance to offset their implementation cost.

The point is that the "regulation" (= semver) already exists.

(I don't believe tooling should be a prerequisite but I'd also point out creating one is straightforward thanks to JuliaFormatter.jl. Here's my POC I experimented a while ago for automatically generating using Foo: f style imports: https://github.com/tkf/AutoImports.jl)

@palday
Copy link
Contributor

palday commented Apr 18, 2024

Firstly, the code does not work anymore. Code should continue to work in new julia releases.

This seems like an argument against adding new exports to Base.

@LilithHafner LilithHafner added the status:triage This should be discussed on a triage call label Apr 18, 2024
@KristofferC
Copy link
Sponsor Member Author

This seems like an argument against adding new exports to Base.

(and all packages).

@MasonProtter
Copy link
Contributor

This is such a silly and tired argument at this point. Can we just move forwards with this PR and/or #53428 ?

DilumAluthge and others added 2 commits May 24, 2024 19:34
Note: the base branch (target branch) of this PR is `kc/warn_using`
(#42080), NOT `master`.

---

This is additive to #42080 which
adds advice to the docstring of `using` .

The wording here is a tweaked version of @IanButterworth's suggestion
here:
#42080 (comment)

---------

Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
@DilumAluthge
Copy link
Member

I've merged #53428 into this PR.

base/docs/basedocs.jl Outdated Show resolved Hide resolved
base/docs/basedocs.jl Outdated Show resolved Hide resolved
@mbauman
Copy link
Sponsor Member

mbauman commented Jun 4, 2024

Well I do not think that we will gather 100% unanimous support here. However, it's worth noting that all the concrete objections here have been addressed since this was originally proposed three years ago:

  • The annotation has been softened from its original negative guidance "using not recommended" to a positive recommendation in favor of "fully-qualified usings "
  • There is now tooling to help migrate codebases to explicit imports (https://github.com/ericphanson/ExplicitImports.jl)
  • The public keyword helps address the what-is-API-through-dot-access question

I think the recommendation (and associated information it contains) is highly valuable, even if you don't agree that this is problematic or worth incorporating into your own personal code style. I fully support a merge at this point.

@mbauman mbauman changed the title recommend against using Foo in package code (or "using considered harmful") recommend explicit using Foo: Foo, ... in package code (was: "using considered harmful") Jun 4, 2024
@fredrikekre
Copy link
Member

There is now tooling to help migrate codebases to explicit imports (https://github.com/ericphanson/ExplicitImports.jl)

Perhaps add a reference to this package? I have found it stable enough and even enabled it in CI for some packages to catch mistakes.

@LilithHafner
Copy link
Member

My approving review indicates that the content itself is well written and mergable. I'm still ambivalent about the sentiment.

We should discuss this at the next triage call in about 42 hours.

@mbauman mbauman requested a review from ararslan June 4, 2024 20:56
Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely better than the initial version of the PR. I made some additional suggestions in-line, but the part I think is very important to note that's currently unstated is that it's not an error if two or more modules export the same binding. Doing so is a common pattern across the ecosystem, so this is potentially confusing as written for new users.

base/docs/basedocs.jl Outdated Show resolved Hide resolved
doc/src/manual/modules.md Outdated Show resolved Hide resolved
@LilithHafner
Copy link
Member

Triage thinks that a good pragmatic solution here is to accept @ararslan's suggestions and merge.

KristofferC and others added 2 commits June 7, 2024 15:08
Co-authored-by: Alex Arslan <ararslan@comcast.net>
Co-authored-by: Alex Arslan <ararslan@comcast.net>
Comment on lines +171 to +179
Qualifying the names being used as in `using Foo: Foo, f` is
recommended over plain `using Foo` for released packages, and other
code which is meant to be re-used in the future with updated dependencies
or future versions of julia.

The reason for this is if another dependency starts to export one of the
same names as `Foo` and you attempt to use that name, then previously working
code will error due to an ambiguity in which package the name should be
taken from.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synchronizing the wording between the docstring and the manual

Suggested change
Qualifying the names being used as in `using Foo: Foo, f` is
recommended over plain `using Foo` for released packages, and other
code which is meant to be re-used in the future with updated dependencies
or future versions of julia.
The reason for this is if another dependency starts to export one of the
same names as `Foo` and you attempt to use that name, then previously working
code will error due to an ambiguity in which package the name should be
taken from.
When two or more packages/modules export a name and that name does not refer to the
same thing in each of the packages, and the packages are loaded via `using` without
an explicit list of names, it is an error to reference that name without qualification.
It is thus recommended that code intended to be forward-compatible with future versions
of its dependencies and of Julia, e.g. code in released packages, list the names it
uses from each loaded package, e.g. `using Foo: Foo, f` rather than `using Foo`.

@Keno Keno mentioned this pull request Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:docs This change adds or pertains to documentation domain:packages Package management and loading modules status:triage This should be discussed on a triage call
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet