Skip to content

Allow using LongPackageName as LPN and change export conventions. #52821

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

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

Conversation

LilithHafner
Copy link
Member

@LilithHafner LilithHafner commented Jan 8, 2024

Before:

julia> using Statistics as Stat
ERROR: ParseError:
# Error @ REPL[1]:1:6
using Statistics as Stat
#    └─────────────────┘ ── `using` with `as` renaming requires a `:` and context module
Stacktrace:
 [1] top-level scope
   @ none:1

After:

julia> using Statistics as Stat

julia> Statistics
ERROR: UndefVarError: `Statistics` not defined in `Main`
Suggestion: check for spelling errors or missing imports.
Hint: a global variable of this name also exists in Statistics.

julia> Stat
Statistics

julia> mean
mean (generic function with 6 methods)

This PR is implemented by

  • No longer automatically export module names
  • Explicitly loading module names on using (i.e. creates an explicit imported const binding)
  • On using Mod as M, add Mod to the list of "used" modules and create an explicit imported binding M => Mod.

(non)breaking change 1

  • Base.isexported(Statistics, :Statistics) will now return false (in Julia <=1.10 Base.isexported was not part of the public API)
  • Base.ispublic(Statistics, :Statistics) will now return false (in Julia <=1.10 Base.public was not defined)

Breaking change 2

names(Mod) currently no longer includes :Mod, I'd like to stop implicitly marking the module itself as public because I don't think Statistics.Statistics is something that should be public by default. Specifically, I think that in the package M1,

module M1
    public API
    const API = M2
    module M2
        public f
        f() = 0
    end
end

The code M1.API.M2.f() depends on internals (the name of the module M2, which was never marked public)

Breaking change 3

The code

julia> module M1
           module M2
           end
       end
Main.M1

julia> module M2
           using ..M1.M2
       end
Main.M2

julia> M2.M2 === M1.M2
false

julia> M2.M2 === M2
true

now throws on

julia> module M1
           module M2
           end
       end
Main.M1

julia> module M2
           using ..M1.M2
       end
ERROR: invalid redefinition of constant M2.M2

because the using statement tries and fails to create an explicit binding. This is a code pattern that is currently in use in Pkg.jl, but I do think it's a bit of an antipattern, and this PR creates a great workaround. However, maybe we should downgrade that into a warning.

Implements #52784 (resolves #52784)
Fixes #52817, Fixes #38521
Is a superset of and therefore closes #52810 (NFC)

Depends on JuliaLang/JuliaSyntax.jl#406
Depends on JuliaLang/Pkg.jl#3750

@LilithHafner LilithHafner added needs tests Unit tests are required for this change minor change Marginal behavior change acceptable for a minor release needs pkgeval Tests for all registered packages should be run with this change labels Jan 8, 2024
@LilithHafner LilithHafner added the needs news A NEWS entry is required for this change label Jan 8, 2024
@LilithHafner LilithHafner removed the needs tests Unit tests are required for this change label Jan 8, 2024
@LilithHafner
Copy link
Member Author

I'm open to more test cases, this is surprisingly finicky.

@LilithHafner LilithHafner added needs docs Documentation for this change is required and removed needs news A NEWS entry is required for this change labels Jan 8, 2024
@LilithHafner LilithHafner marked this pull request as ready for review January 8, 2024 22:01
@LilithHafner
Copy link
Member Author

LilithHafner commented Feb 5, 2024

Dropping this link here as a TODO once we update JuliaSyntax and unblock this PR: review this discord thread and consider if/how the changes here impact that situation. EDIT: this PR would help that situation a wee bit but not address it much.

@LilithHafner LilithHafner added the triage This should be discussed on a triage call label Mar 25, 2025
@LilithHafner
Copy link
Member Author

Tagging triage because this would be a significant new syntax feature.

@StefanKarpinski
Copy link
Member

I like this feature and think we should have it. Someone needs to implement.

@LilithHafner LilithHafner removed the triage This should be discussed on a triage call label Apr 10, 2025
@StefanKarpinski
Copy link
Member

Triage likes it. My argument in favor of making using Foo create an explicit const binding for Foo is that the implicit bindings are for things that you didn't explicitly ask for, but you did explicitly ask for Foo! Of course, as it stands, you might have a situation where you want the things exported by Foo but want to use the name Foo for something else... enter using Foo as F which lets you pick a different name for Foo so that you can use the Foo name yourself. So it makes sense for these changes to go together. @xal-0 noted that this eliminates an annoying special case for the Main module, which is a nice side effect.

One additional feature I'd like to see is that using Foo as _ skips creating a binding for Foo altogether.

@DilumAluthge
Copy link
Member

What is the intended use case here? In the REPL, or in package code?

Stdlib: Pkg
URL: https://github.com/JuliaLang/Pkg.jl.git
Stdlib branch: master
Julia branch: lh/using-as
Old commit: 5432fdd30
New commit: 4c9b08431
Julia version: 1.13.0-DEV
Pkg version: 1.12.0(Does not match)
Bump invoked by: @LilithHafner
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/Pkg.jl@5432fdd...4c9b084

```
$ git log --oneline 5432fdd30..4c9b08431
4c9b08431 Stop using the `module Mod; using Mod; end` pattern [NFC] (#3750)
bde7ce039 also collect packages tracking a repo when "traversing" developed packages (#4198)
```

Co-authored-by: LilithHafner <60898866+LilithHafner@users.noreply.github.com>
@LilithHafner
Copy link
Member Author

Both, same as import Mod as Alt.

@DilumAluthge
Copy link
Member

DilumAluthge commented Apr 11, 2025

But (unlike import Foo as Bar), this new syntax will bring all the exported names into scope, right?

I’m not thrilled about this. I don’t really think we should add new syntax that encourages people to disregard #42080 in package code.

@DilumAluthge
Copy link
Member

Also, FWIW, it wasn’t immediately obvious to me at first (just from looking at the syntax) that using Foo as Bar brings all the exported names into scope. So in theory that’s another potential footgun (people not realizing that they’re bringing all those names into scope).

But that may just be me personally.

I think I’m more concerned about the #42080 aspect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor change Marginal behavior change acceptable for a minor release needs docs Documentation for this change is required needs pkgeval Tests for all registered packages should be run with this change
Projects
None yet
4 participants