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

Add a warning for auto-import of types #57311

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Add a warning for auto-import of types #57311

merged 1 commit into from
Feb 10, 2025

Conversation

Keno
Copy link
Member

@Keno Keno commented Feb 7, 2025

This adds a warning for the auto-import of types cases (#25744) that we have long considered a bit of a bug, but didn't want to change because it is too breaking.

The reason to do it now is that the binding rework has made this case more problematic (see #57290). To summarize, the question is what happens when the compiler sees f(x) = ... and f is currently and implicitly imported binding. There are two options:

  1. We add a method to the generic function referred to by f, or
  2. We create a new generic function f in the current module.

Historically, case 1 has the additional complication that this error'd unless f is a type. It is my impression that a lot of existing code did not have a particularly good understanding of the resolved-ness dependence of this behavior.

However, because case 1 errors for generic functions, it appears that existing code generally expects case 2. On the other hand, for types, there is existing code in both directions (#57290 is an example of case 2; see #57302 for examples of case 1). That said, case 1 is more common (because types tend to be resolved because they're used in signatures at toplevel).

Thus, to retain compatibility, the current behavior on master (where resolvedness is no longer available) is that we always choose case 2 for functions and case 1 for types. This inconsistency is unfortunate, but I tried resolving this in either way (making all situations case 1 or all case 2) and the result was too breaking.

Nevertheless, it is problematic that there is existing code that expects case 2 beavior for types and we should help users to know what the correct way to fix it is. The proposed resolution is thus:

  1. Retain case 1 behavior for types
  2. Make it a warning to use, encouraging people to explicitly import, since we generally consider the Extending a constructor is possible without explicit import or module prefix #25744 case a bug.

Example:

julia> module Foo
         String(i::Int) = i
       end
WARNING: Type Core.String was auto-`import`ed in `Foo`.
NOTE: This behavior is deprecated and may change in future Julia versions.
NOTE: This behavior may have differed in Julia versions prior to 1.12 depending on binding resolution.
Hint: To retain the current behavior, add an explicit `import Core: String` in Foo.
Hint: To create a new generic function of the same name use `function String end`.
Main.Foo

@Keno Keno force-pushed the kf/57290 branch 2 times, most recently from 4b55689 to 4c0bff4 Compare February 8, 2025 00:04
Keno added a commit to JuliaLang/LinearAlgebra.jl that referenced this pull request Feb 8, 2025
These would otherwise give a warning after JuliaLang/julia#57311, but are a good change even independently.
Keno added a commit to JuliaSparse/SparseArrays.jl that referenced this pull request Feb 8, 2025
These would otherwise give a warning after JuliaLang/julia#57311, but are a good change even independently.
@Keno Keno added the backport 1.12 Change should be backported to release-1.12 label Feb 8, 2025
@Keno Keno force-pushed the kf/57290 branch 2 times, most recently from 045c5c0 to fad2f2a Compare February 8, 2025 01:05
src/module.c Outdated
jl_errorf("invalid method definition in %s: function %s.%s must be explicitly imported to be extended",
jl_module_debug_name(m), jl_module_debug_name(from), jl_symbol_name(var));
else
jl_printf(JL_STDERR, "WARNING: Type %s.%s was auto-`import`ed in `%s`.\n"
Copy link
Member

Choose a reason for hiding this comment

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

If I'm a user who wrote String(x) = 1 or whatever and got this warning, I don't think I'd make the connection that the problem was actually because I was extending a type constructor without qualification. Can that be made more explicit? Also, it could be worthwhile to encourage qualification for extension in the message rather than (or at least in addition to) implicit extension via import.

Copy link
Member

Choose a reason for hiding this comment

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

Here's a stab at another message:

WARNING: Defining a method on implicitly-imported type "String" is deprecated.
         Assumed "String" refers to `Core.String`. This behavior may have differed in Julia versions prior to 1.12.
         To fix this warning:
           If you intended to refer to `Core.String`, add an explicit `import Core: String` in Foo.
           If you wanted to create a new generic function of the same name, use `function String end`.

I don't think it's quite there yet, but hopefully it gives someone ideas for refinement

@DilumAluthge
Copy link
Member

This sounds like another candidate for "strict mode"?

ViralBShah pushed a commit to JuliaSparse/SparseArrays.jl that referenced this pull request Feb 8, 2025
These would otherwise give a warning after JuliaLang/julia#57311, but
are a good change even independently.
@Keno
Copy link
Member Author

Keno commented Feb 10, 2025

I'll take a slight tweak on @topolarity's message above, and then we can merge this and play with further tweaks to the message in follow-on PRs.

@Keno
Copy link
Member Author

Keno commented Feb 10, 2025

This sounds like another candidate for "strict mode"?

Yes, I had suggested a strict mode option that would require all generic functions that have the same name as an implicit import to be explicitly declared using function foo end.

Keno added a commit to JuliaLang/LinearAlgebra.jl that referenced this pull request Feb 10, 2025
These would otherwise give a warning after
JuliaLang/julia#57311, but are a good change
even independently.

---------

Co-authored-by: Daniel Karrasch <[email protected]>
This adds a warning for the auto-import of types cases (#25744) that
we have long considered a bit of a bug, but didn't want to change because
it is too breaking.

The reason to do it now is that the binding rework has made this case
more problematic (see #57290). To summarize, the question is what
happens when the compiler sees `f(x) = ...` and `f` is currently
and implicitly imported binding. There are two options:

1. We add a method to the generic function referred to by `f`, or
2. We create a new generic function `f` in the current module.

Historically, case 1 has the additional complication that this
error'd unless `f` is a type. It is my impression that a lot
of existing code did not have a particularly good understanding
of the resolved-ness dependence of this behavior.

However, because case 1 errors for generic functions, it appears
that existing code generally expects case 2. On the other hand,
for types, there is existing code in both directions (#57290 is
an example of case 2; see #57302 for examples of case 1). That
said, case 1 is more common (because types tend to be resolved
because they're used in signatures at toplevel).

Thus, to retain compatibility, the current behavior on master (where
resolvedness is no longer available) is that we always choose
case 2 for functions and case 1 for types. This inconsistency is
unfortunate, but I tried resolving this in either way (making
all situations case 1 or all case 2) and the result was too breaking.

Nevertheless, it is problematic that there is existing code that
expects case 2 beavior for types and we should help users to know
what the correct way to fix it is. The proposed resolution is thus:
1. Retain case 1 behavior for types
2. Make it a warning to use, encouraging people to explicitly import,
   since we generally consider the #25744 case a bug.

Example:
```
julia> module Foo
         String(i::Int) = i
       end
WARNING: Type Core.String was auto-`import`ed in `Foo`.
NOTE: This behavior is deprecated and may change in future Julia versions.
NOTE: This behavior may have differed in Julia versions prior to 1.12 depending on binding resolution.
Hint: To retain the current behavior, add an explicit `import Core: String` in Foo.
Hint: To create a new generic function of the same name use `function String end`.
Main.Foo
```
@Keno Keno merged commit 8c62f42 into master Feb 10, 2025
5 of 7 checks passed
@Keno Keno deleted the kf/57290 branch February 10, 2025 13:31
@lgoettgens
Copy link
Contributor

lgoettgens commented Feb 10, 2025

I just ran this on one of the packages I work on (oscar-system/GAP.jl), and this seem to report all instances double during precompilation (sorry for not doing this before merge):

julia +pr57311 --project=.
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.13.0-DEV.26 (2025-02-10)
 _/ |\__'_|_|_|\__'_|  |  kf/57290/294b765d6c9 (fork: 1 commits, 0 days)
|__/                   |

julia> using GAP
Info Given GAP was explicitly requested, output will be shown live 
WARNING: Constructor for type "String" was extended in `GAP` without explicit qualification or import.
  NOTE: Assumed "String" refers to `Core.String`. This behavior is deprecated and may differ in future versions.`
  NOTE: This behavior may have differed in Julia versions prior to 1.12.
  Hint: If you intended to create a new generic function of the same name, use `function String end`.
  Hint: To silence the warning, qualify `String` as `Core.String` or explicitly `import Core: String`
WARNING: Constructor for type "String" was extended in `GAP` without explicit qualification or import.
  NOTE: Assumed "String" refers to `Core.String`. This behavior is deprecated and may differ in future versions.`
  NOTE: This behavior may have differed in Julia versions prior to 1.12.
  Hint: If you intended to create a new generic function of the same name, use `function String end`.
  Hint: To silence the warning, qualify `String` as `Core.String` or explicitly `import Core: String`
WARNING: Constructor for type "BitVector" was extended in `GAP` without explicit qualification or import.
  NOTE: Assumed "BitVector" refers to `Base.BitVector`. This behavior is deprecated and may differ in future versions.`
  NOTE: This behavior may have differed in Julia versions prior to 1.12.
  Hint: If you intended to create a new generic function of the same name, use `function BitVector end`.
  Hint: To silence the warning, qualify `BitVector` as `Base.BitVector` or explicitly `import Base: BitVector`
WARNING: Constructor for type "BitVector" was extended in `GAP` without explicit qualification or import.
  NOTE: Assumed "BitVector" refers to `Base.BitVector`. This behavior is deprecated and may differ in future versions.`
  NOTE: This behavior may have differed in Julia versions prior to 1.12.
  Hint: If you intended to create a new generic function of the same name, use `function BitVector end`.
  Hint: To silence the warning, qualify `BitVector` as `Base.BitVector` or explicitly `import Base: BitVector`
Precompiling GAP finished.
  1 dependency successfully precompiled in 45 seconds. 92 already precompiled.
  1 dependency had output during precompilation:
┌ GAP
│  [Output was shown above]

 ┌───────┐   GAP 4.14.0 of 2024-12-05
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-julia1.13-64-kv9
 Configuration:  gmp 6.3.0, Julia GC, Julia 1.13.0-DEV, readline
 Loading the library and packages ...
 Packages:   AClib 1.3.2, Alnuth 3.2.1, AtlasRep 2.1.9, AutPGrp 1.11, Browse 1.8.21, CaratInterface 2.3.7, CRISP 1.4.6, 
             Cryst 4.1.27, CrystCat 1.1.10, CTblLib 1.3.9, curlInterface 2.4.0, FactInt 1.6.3, FGA 1.5.0, Forms 1.2.12, 
             GAPDoc 1.6.7, genss 1.6.9, IO 4.9.1, IRREDSOL 1.4.4, JuliaInterface 0.13.1, LAGUNA 3.9.7, orb 4.9.1, 
             Polenta 1.3.10, Polycyclic 2.16, PrimGrp 3.4.4, RadiRoot 2.9, recog 1.4.3, ResClasses 4.7.3, SmallGrp 1.5.4, 
             Sophus 1.27, SpinSym 1.5.2, StandardFF 1.0, TomLib 1.2.11, TransGrp 3.6.5, utils 0.85
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'

julia> 

@Keno
Copy link
Member Author

Keno commented Feb 10, 2025

Yes, there's a lowering bug that tries to create every function twice. It's on my list of things to fix.

@topolarity
Copy link
Member

Is it possible to add the file + line no. to the WARNING?

@topolarity
Copy link
Member

Also this may be minor, but it's slightly unfortunate that this encourages users to write Core.Float64 instead of Base.Float64. For the most part, I think users don't have to worry about the existence of Core so this adds some noise. That its bindings are heavily aliased with Base is pretty confusing, too.

Is it possible to check if the type is available in Base and use that as the module name instead?

Keno added a commit that referenced this pull request Feb 10, 2025
Before:
```
:($(Expr(:thunk, CodeInfo(
1 ─       $(Expr(:thunk, CodeInfo(
1 ─     return $(Expr(:method, :(Main.f)))
)))
│         $(Expr(:method, :(Main.f)))
│   %3  = Main.f
│   %4  =   dynamic Core.Typeof(%3)
│   %5  =   builtin Core.svec(%4, Core.Any)
│   %6  =   builtin Core.svec()
│   %7  =   builtin Core.svec(%5, %6, $(QuoteNode(:(#= REPL[2]:1 =#))))
│         $(Expr(:method, :(Main.f), :(%7), CodeInfo(
1 ─     return 1
)))
│         $(Expr(:latestworld))
│   %10 = Main.f
└──       return %10
))))
```

After:
```
julia> @Meta.lower f(x)=1
:($(Expr(:thunk, CodeInfo(
1 ─       $(Expr(:method, :(Main.f)))
│         $(Expr(:latestworld))
│         Main.f
│         $(Expr(:latestworld))
│   %5  = Main.f
│   %6  =   dynamic Core.Typeof(%5)
│   %7  =   builtin Core.svec(%6, Core.Any)
│   %8  =   builtin Core.svec()
│   %9  =   builtin Core.svec(%7, %8, $(QuoteNode(:(#= REPL[1]:1 =#))))
│         $(Expr(:method, :(Main.f), :(%9), CodeInfo(
1 ─     return 1
)))
│         $(Expr(:latestworld))
│   %12 = Main.f
└──       return %12
))))
```

This doesn't really make a semantic difference, but if `f` is a type,
we may now give a warning, so the prior definition would give the
warning twice (#57311 (comment)).
We may want to consider rate-limiting the warning independently,
but for now at least give the correct number of warnings.
@Keno
Copy link
Member Author

Keno commented Feb 10, 2025

Is it possible to add the file + line no. to the WARNING?

We can give the toplevel location, but we generally avoid it, because it's misleading if you're getting there through an eval. My plan was to hook both of the new warnings up to --depwarn=error and let people get backtraces that way. We don't generally have access to full backtraces in warnings. We could try to collect a backtrace, but all the printing for them is julia-side, so it's a bit complicated.

Is it possible to check if the type is available in Base and use that as the module name instead?

It prints the module that the type was implicitly import'ed from. We could stop exporting these from Core, we could hardcode Core as lower priority, we could reverse the order of the imports or we could come up with some sort of generally applicable principle for how to resolve the imports if they come from multiple modules (although I would suspect that such a mechanism would prefer Core since it's the original source and they're both implicitly using'd).

KristofferC pushed a commit that referenced this pull request Feb 11, 2025
This adds a warning for the auto-import of types cases (#25744) that we
have long considered a bit of a bug, but didn't want to change because
it is too breaking.

The reason to do it now is that the binding rework has made this case
more problematic (see #57290). To summarize, the question is what
happens when the compiler sees `f(x) = ...` and `f` is currently and
implicitly imported binding. There are two options:

1. We add a method to the generic function referred to by `f`, or
2. We create a new generic function `f` in the current module.

Historically, case 1 has the additional complication that this error'd
unless `f` is a type. It is my impression that a lot of existing code
did not have a particularly good understanding of the resolved-ness
dependence of this behavior.

However, because case 1 errors for generic functions, it appears that
existing code generally expects case 2. On the other hand, for types,
there is existing code in both directions (#57290 is an example of case
2; see #57302 for examples of case 1). That said, case 1 is more common
(because types tend to be resolved because they're used in signatures at
toplevel).

Thus, to retain compatibility, the current behavior on master (where
resolvedness is no longer available) is that we always choose case 2 for
functions and case 1 for types. This inconsistency is unfortunate, but I
tried resolving this in either way (making all situations case 1 or all
case 2) and the result was too breaking.

Nevertheless, it is problematic that there is existing code that expects
case 2 beavior for types and we should help users to know what the
correct way to fix it is. The proposed resolution is thus:
1. Retain case 1 behavior for types
2. Make it a warning to use, encouraging people to explicitly import,
since we generally consider the #25744 case a bug.

Example:
```
julia> module Foo
         String(i::Int) = i
       end
WARNING: Type Core.String was auto-`import`ed in `Foo`.
NOTE: This behavior is deprecated and may change in future Julia versions.
NOTE: This behavior may have differed in Julia versions prior to 1.12 depending on binding resolution.
Hint: To retain the current behavior, add an explicit `import Core: String` in Foo.
Hint: To create a new generic function of the same name use `function String end`.
Main.Foo
```

(cherry picked from commit 8c62f42)
@KristofferC KristofferC mentioned this pull request Feb 11, 2025
32 tasks
Keno added a commit that referenced this pull request Feb 11, 2025
Inspired by the question in #57311 (comment),
I want to revisit the basic setup where every module `using`s both
`Core` and `Base`. In general, I think we mostly expect users to
inferface with `Base`, not `Core`, so this PR changes things to
only have new modules `using` Base (while re-exporting all `Core`
names from `Base`). There should be little user-visible impact
from these changes. The only situation I can think of where it
makes a difference is if somebody were to make their own Base/toplevel
module that does not re-export Core. However, we don't really support
that situation in the first place, and I actually think it's a feature
that such toplevel modules can more closely control the set of implicitly
exposed names.
@topolarity
Copy link
Member

We can give the toplevel location, but we generally avoid it, because it's misleading if you're getting there through an eval.

Do we know when we got there through an eval or not? I'd be fine if you only get the location when not eval()'ing and then rely on --depwarn=error for the rest

@Keno
Copy link
Member Author

Keno commented Feb 11, 2025

Not really. We just store the last time you encountered a LineNumberNode at toplevel.

Keno added a commit that referenced this pull request Feb 11, 2025
Inspired by the question in #57311 (comment),
I want to revisit the basic setup where every module `using`s both
`Core` and `Base`. In general, I think we mostly expect users to
inferface with `Base`, not `Core`, so this PR changes things to
only have new modules `using` Base (while re-exporting all `Core`
names from `Base`). There should be little user-visible impact
from these changes. The only situation I can think of where it
makes a difference is if somebody were to make their own Base/toplevel
module that does not re-export Core. However, we don't really support
that situation in the first place, and I actually think it's a feature
that such toplevel modules can more closely control the set of implicitly
exposed names.
Keno added a commit that referenced this pull request Feb 11, 2025
Before:
```
:($(Expr(:thunk, CodeInfo(
1 ─       $(Expr(:thunk, CodeInfo(
1 ─     return $(Expr(:method, :(Main.f)))
)))
│         $(Expr(:method, :(Main.f)))
│   %3  = Main.f
│   %4  =   dynamic Core.Typeof(%3)
│   %5  =   builtin Core.svec(%4, Core.Any)
│   %6  =   builtin Core.svec()
│   %7  =   builtin Core.svec(%5, %6, $(QuoteNode(:(#= REPL[2]:1 =#))))
│         $(Expr(:method, :(Main.f), :(%7), CodeInfo(
1 ─     return 1
)))
│         $(Expr(:latestworld))
│   %10 = Main.f
└──       return %10
))))
```

After:
```
julia> @Meta.lower f(x)=1
:($(Expr(:thunk, CodeInfo(
1 ─       $(Expr(:method, :(Main.f)))
│         $(Expr(:latestworld))
│         Main.f
│         $(Expr(:latestworld))
│   %5  = Main.f
│   %6  =   dynamic Core.Typeof(%5)
│   %7  =   builtin Core.svec(%6, Core.Any)
│   %8  =   builtin Core.svec()
│   %9  =   builtin Core.svec(%7, %8, $(QuoteNode(:(#= REPL[1]:1 =#))))
│         $(Expr(:method, :(Main.f), :(%9), CodeInfo(
1 ─     return 1
)))
│         $(Expr(:latestworld))
│   %12 = Main.f
└──       return %12
))))
```

This doesn't really make a semantic difference, but if `f` is a type,
we may now give a warning, so the prior definition would give the
warning twice (#57311 (comment)).
We may want to consider rate-limiting the warning independently,
but for now at least give the correct number of warnings.
Keno added a commit that referenced this pull request Feb 11, 2025
Before:
```
:($(Expr(:thunk, CodeInfo(
1 ─       $(Expr(:thunk, CodeInfo(
1 ─     return $(Expr(:method, :(Main.f)))
)))
│         $(Expr(:method, :(Main.f)))
│   %3  = Main.f
│   %4  =   dynamic Core.Typeof(%3)
│   %5  =   builtin Core.svec(%4, Core.Any)
│   %6  =   builtin Core.svec()
│   %7  =   builtin Core.svec(%5, %6, $(QuoteNode(:(#= REPL[2]:1 =#))))
│         $(Expr(:method, :(Main.f), :(%7), CodeInfo(
1 ─     return 1
)))
│         $(Expr(:latestworld))
│   %10 = Main.f
└──       return %10
))))
```

After:
```
julia> @Meta.lower f(x)=1
:($(Expr(:thunk, CodeInfo(
1 ─       $(Expr(:method, :(Main.f)))
│         $(Expr(:latestworld))
│         Main.f
│         $(Expr(:latestworld))
│   %5  = Main.f
│   %6  =   dynamic Core.Typeof(%5)
│   %7  =   builtin Core.svec(%6, Core.Any)
│   %8  =   builtin Core.svec()
│   %9  =   builtin Core.svec(%7, %8, $(QuoteNode(:(#= REPL[1]:1 =#))))
│         $(Expr(:method, :(Main.f), :(%9), CodeInfo(
1 ─     return 1
)))
│         $(Expr(:latestworld))
│   %12 = Main.f
└──       return %12
))))
```

This doesn't really make a semantic difference, but if `f` is a type,
we may now give a warning, so the prior definition would give the
warning twice (#57311 (comment)).
We may want to consider rate-limiting the warning independently,
but for now at least give the correct number of warnings.
Keno added a commit that referenced this pull request Feb 12, 2025
Inspired by the question in #57311 (comment),
I want to revisit the basic setup where every module `using`s both
`Core` and `Base`. In general, I think we mostly expect users to
inferface with `Base`, not `Core`, so this PR changes things to
only have new modules `using` Base (while re-exporting all `Core`
names from `Base`). There should be little user-visible impact
from these changes. The only situation I can think of where it
makes a difference is if somebody were to make their own Base/toplevel
module that does not re-export Core. However, we don't really support
that situation in the first place, and I actually think it's a feature
that such toplevel modules can more closely control the set of implicitly
exposed names.
vtjnash added a commit that referenced this pull request Feb 12, 2025
Quiets the build somewhat, after #57311 made it noisy.
Keno added a commit that referenced this pull request Feb 12, 2025
Before:
```
:($(Expr(:thunk, CodeInfo(
1 ─       $(Expr(:thunk, CodeInfo(
1 ─     return $(Expr(:method, :(Main.f)))
)))
│         $(Expr(:method, :(Main.f)))
│   %3  = Main.f
│   %4  =   dynamic Core.Typeof(%3)
│   %5  =   builtin Core.svec(%4, Core.Any)
│   %6  =   builtin Core.svec()
│   %7  =   builtin Core.svec(%5, %6, $(QuoteNode(:(#= REPL[2]:1 =#))))
│         $(Expr(:method, :(Main.f), :(%7), CodeInfo(
1 ─     return 1
)))
│         $(Expr(:latestworld))
│   %10 = Main.f
└──       return %10
))))
```

After:
```
julia> @Meta.lower f(x)=1
:($(Expr(:thunk, CodeInfo(
1 ─       $(Expr(:method, :(Main.f)))
│         $(Expr(:latestworld))
│         Main.f
│         $(Expr(:latestworld))
│   %5  = Main.f
│   %6  =   dynamic Core.Typeof(%5)
│   %7  =   builtin Core.svec(%6, Core.Any)
│   %8  =   builtin Core.svec()
│   %9  =   builtin Core.svec(%7, %8, $(QuoteNode(:(#= REPL[1]:1 =#))))
│         $(Expr(:method, :(Main.f), :(%9), CodeInfo(
1 ─     return 1
)))
│         $(Expr(:latestworld))
│   %12 = Main.f
└──       return %12
))))
```

This doesn't really make a semantic difference, but if `f` is a type, we
may now give a warning, so the prior definition would give the warning
twice
(#57311 (comment)).
We may want to consider rate-limiting the warning independently, but for
now at least give the correct number of warnings.
Keno added a commit that referenced this pull request Feb 12, 2025
Inspired by the question in #57311 (comment),
I want to revisit the basic setup where every module `using`s both
`Core` and `Base`. In general, I think we mostly expect users to
inferface with `Base`, not `Core`, so this PR changes things to
only have new modules `using` Base (while re-exporting all `Core`
names from `Base`). There should be little user-visible impact
from these changes. The only situation I can think of where it
makes a difference is if somebody were to make their own Base/toplevel
module that does not re-export Core. However, we don't really support
that situation in the first place, and I actually think it's a feature
that such toplevel modules can more closely control the set of implicitly
exposed names.
Keno added a commit that referenced this pull request Feb 13, 2025
Inspired by the question in
#57311 (comment), I
want to revisit the basic setup where every module `using`s both `Core`
and `Base`. In general, I think we mostly expect users to inferface with
`Base`, not `Core`, so this PR changes things to only have new modules
`using` Base (while re-exporting all `Core` names from `Base`). There
should be little user-visible impact from these changes. The only
situation I can think of where it makes a difference is if somebody were
to make their own Base/toplevel module that does not re-export Core.
However, we don't really support that situation in the first place, and
I actually think it's a feature that such toplevel modules can more
closely control the set of implicitly exposed names.
vtjnash added a commit that referenced this pull request Feb 13, 2025
Quiets the build somewhat, after #57311 made it noisy.
vtjnash added a commit that referenced this pull request Feb 13, 2025
Quiets the build somewhat, after #57311 made it noisy.
KristofferC added a commit that referenced this pull request Feb 13, 2025
Backported PRs:
- [x] #57142 <!-- Add reference to time_ns in time -->
- [x] #57241 <!-- Handle `waitpid` race condition when `SIGCHLD` is set
to `SIG_IGN` -->
- [x] #57249 <!-- restore non-freebsd-unix fix for profiling -->
- [x] #57211 <!-- Ensure read/readavailable for BufferStream are
threadsafe -->
- [x] #57262 <!-- edit NEWS for v1.12 -->
- [x] #57226 <!-- cfunction: reimplement, as originally planned, for
reliable performance -->
- [x] #57253 <!-- bpart: Fully switch to partitioned semantics -->
- [x] #57273 <!-- fix "Right arrow autocompletes at line end"
implementation -->
- [x] #57280 <!-- dep: Update JuliaSyntax -->
- [x] #57229 <!-- staticdata: Close data race after backedge insertion
-->
- [x] #57298 <!-- Updating binding version to fix MMTk CI -->
- [x] #57248 <!-- improve concurrency safety for `Compiler.finish!` -->
- [x] #57312 <!-- Profile.print: de-focus sleeping frames as gray -->
- [x] #57289 <!-- Make `OncePerX` subtype `Function` -->
- [x] #57310 <!-- Make ptls allocations at least 128 byte aligned -->
- [x] #57311 <!-- Add a warning for auto-import of types -->
- [x] #57338 <!-- fix typo in Float32 random number generation -->
- [x] #57293 <!-- Fix getfield_tfunc when order or boundscheck is Vararg
-->
- [x] #57349 <!-- docs: fix-up world-age handling for META access -->
- [x] #57344 <!-- Add missing type asserts when taking the queue out of
the task struct -->
- [x] #57348 <!-- 🤖 [master] Bump the SparseArrays stdlib from 212981b
to 72c7cac -->
- [x] #55040 <!-- Allow macrocall as function sig -->
- [x] #57299 <!-- Add missing latestworld after parameterized type alias
-->
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Feb 13, 2025
KristofferC pushed a commit that referenced this pull request Feb 14, 2025
Quiets the build somewhat, after #57311 made it noisy.

(cherry picked from commit 6b39a81)
KristofferC pushed a commit that referenced this pull request Feb 14, 2025
Before:
```
:($(Expr(:thunk, CodeInfo(
1 ─       $(Expr(:thunk, CodeInfo(
1 ─     return $(Expr(:method, :(Main.f)))
)))
│         $(Expr(:method, :(Main.f)))
│   %3  = Main.f
│   %4  =   dynamic Core.Typeof(%3)
│   %5  =   builtin Core.svec(%4, Core.Any)
│   %6  =   builtin Core.svec()
│   %7  =   builtin Core.svec(%5, %6, $(QuoteNode(:(#= REPL[2]:1 =#))))
│         $(Expr(:method, :(Main.f), :(%7), CodeInfo(
1 ─     return 1
)))
│         $(Expr(:latestworld))
│   %10 = Main.f
└──       return %10
))))
```

After:
```
julia> @Meta.lower f(x)=1
:($(Expr(:thunk, CodeInfo(
1 ─       $(Expr(:method, :(Main.f)))
│         $(Expr(:latestworld))
│         Main.f
│         $(Expr(:latestworld))
│   %5  = Main.f
│   %6  =   dynamic Core.Typeof(%5)
│   %7  =   builtin Core.svec(%6, Core.Any)
│   %8  =   builtin Core.svec()
│   %9  =   builtin Core.svec(%7, %8, $(QuoteNode(:(#= REPL[1]:1 =#))))
│         $(Expr(:method, :(Main.f), :(%9), CodeInfo(
1 ─     return 1
)))
│         $(Expr(:latestworld))
│   %12 = Main.f
└──       return %12
))))
```

This doesn't really make a semantic difference, but if `f` is a type, we
may now give a warning, so the prior definition would give the warning
twice
(#57311 (comment)).
We may want to consider rate-limiting the warning independently, but for
now at least give the correct number of warnings.

(cherry picked from commit 512eb5e)
KristofferC pushed a commit that referenced this pull request Feb 14, 2025
Inspired by the question in
#57311 (comment), I
want to revisit the basic setup where every module `using`s both `Core`
and `Base`. In general, I think we mostly expect users to inferface with
`Base`, not `Core`, so this PR changes things to only have new modules
`using` Base (while re-exporting all `Core` names from `Base`). There
should be little user-visible impact from these changes. The only
situation I can think of where it makes a difference is if somebody were
to make their own Base/toplevel module that does not re-export Core.
However, we don't really support that situation in the first place, and
I actually think it's a feature that such toplevel modules can more
closely control the set of implicitly exposed names.

(cherry picked from commit 20162ea)
dlfivefifty pushed a commit to JuliaArrays/LazyArrays.jl that referenced this pull request Feb 15, 2025
* Explicitly import `ArrayLayouts.MemoryLayout`

Prevent name ambiguity, adapt to changes in nightly Julia, preventing a
warning during precompilation.

See:
* JuliaLang/julia#25744
* JuliaLang/julia#57290
* JuliaLang/julia#57311

* apply suggestion
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.

6 participants