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

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

Open
wants to merge 17 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
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

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

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
Development

Successfully merging this pull request may close these issues.

using creates explicit bindings inconsistently Support using Statistics as Stat
1 participant