From 294b765d6c9a5744ff7fd1f56c50f5cbd86aac41 Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Fri, 7 Feb 2025 20:12:25 +0000 Subject: [PATCH] Add a warning for auto-import of types 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 ``` --- src/codegen.cpp | 25 ++++------------------ src/interpreter.c | 3 +-- src/julia.h | 11 ++++++++-- src/julia_internal.h | 1 + src/method.c | 3 ++- src/module.c | 28 ++++++++++++++++++------- stdlib/SharedArrays/src/SharedArrays.jl | 1 + test/arrayops.jl | 2 +- test/errorshow.jl | 2 +- test/intrinsics.jl | 2 +- 10 files changed, 41 insertions(+), 37 deletions(-) diff --git a/src/codegen.cpp b/src/codegen.cpp index 16cedabed107a..b57fc7d90eb29 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -1005,7 +1005,7 @@ static const auto jlgenericfunction_func = new JuliaFunction<>{ auto T_jlvalue = JuliaType::get_jlvalue_ty(C); auto T_pjlvalue = PointerType::get(T_jlvalue, 0); auto T_prjlvalue = PointerType::get(T_jlvalue, AddressSpace::Tracked); - return FunctionType::get(T_prjlvalue, {T_pjlvalue, T_pjlvalue, T_pjlvalue}, false); + return FunctionType::get(T_prjlvalue, {T_pjlvalue, T_pjlvalue}, false); }, nullptr, }; @@ -6853,8 +6853,6 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaidx_ jl_value_t *mn = args[0]; assert(jl_is_symbol(mn) || jl_is_slotnumber(mn) || jl_is_globalref(mn)); - Value *bp = NULL, *name; - jl_binding_t *bnd = NULL; bool issym = jl_is_symbol(mn); bool isglobalref = !issym && jl_is_globalref(mn); jl_module_t *mod = ctx.module; @@ -6863,26 +6861,11 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaidx_ mod = jl_globalref_mod(mn); mn = (jl_value_t*)jl_globalref_name(mn); } - JL_TRY { - if (jl_symbol_name((jl_sym_t*)mn)[0] == '@') - jl_errorf("macro definition not allowed inside a local scope"); - name = literal_pointer_val(ctx, mn); - bnd = jl_get_binding_for_method_def(mod, (jl_sym_t*)mn); - } - JL_CATCH { - jl_value_t *e = jl_current_exception(jl_current_task); - // errors. boo. :( - JL_GC_PUSH1(&e); - e = jl_as_global_root(e, 1); - JL_GC_POP(); - raise_exception(ctx, literal_pointer_val(ctx, e)); - return ghostValue(ctx, jl_nothing_type); - } - bp = julia_binding_gv(ctx, bnd); jl_cgval_t gf = mark_julia_type( ctx, - ctx.builder.CreateCall(prepare_call(jlgenericfunction_func), { bp, - literal_pointer_val(ctx, (jl_value_t*)mod), name + ctx.builder.CreateCall(prepare_call(jlgenericfunction_func), { + literal_pointer_val(ctx, (jl_value_t*)mod), + literal_pointer_val(ctx, (jl_value_t*)mn) }), true, jl_function_type); diff --git a/src/interpreter.c b/src/interpreter.c index 513fe58f7b5cc..7ab284df78dff 100644 --- a/src/interpreter.c +++ b/src/interpreter.c @@ -93,8 +93,7 @@ static jl_value_t *eval_methoddef(jl_expr_t *ex, interpreter_state *s) if (!jl_is_symbol(fname)) { jl_error("method: invalid declaration"); } - jl_binding_t *b = jl_get_binding_for_method_def(modu, fname); - return jl_declare_const_gf(b, modu, fname); + return jl_declare_const_gf(modu, fname); } jl_value_t *atypes = NULL, *meth = NULL, *fname = NULL; diff --git a/src/julia.h b/src/julia.h index dd3bc713a517f..1fd709f42ee31 100644 --- a/src/julia.h +++ b/src/julia.h @@ -1475,6 +1475,13 @@ STATIC_INLINE char *jl_symbol_name_(jl_sym_t *s) JL_NOTSAFEPOINT } #define jl_symbol_name(s) jl_symbol_name_(s) +STATIC_INLINE const char *jl_module_debug_name(jl_module_t *mod) JL_NOTSAFEPOINT +{ + if (!mod) + return ""; + return jl_symbol_name(mod->name); +} + static inline uint32_t jl_fielddesc_size(int8_t fielddesc_type) JL_NOTSAFEPOINT { assert(fielddesc_type >= 0 && fielddesc_type <= 2); @@ -1901,7 +1908,7 @@ JL_DLLEXPORT jl_value_t *jl_get_binding_value(jl_binding_t *b JL_PROPAGATES_ROOT JL_DLLEXPORT jl_value_t *jl_get_binding_value_if_const(jl_binding_t *b JL_PROPAGATES_ROOT); JL_DLLEXPORT jl_value_t *jl_get_binding_value_if_resolved(jl_binding_t *b JL_PROPAGATES_ROOT) JL_NOTSAFEPOINT; JL_DLLEXPORT jl_value_t *jl_get_binding_value_if_resolved_and_const(jl_binding_t *b JL_PROPAGATES_ROOT) JL_NOTSAFEPOINT; -JL_DLLEXPORT jl_value_t *jl_declare_const_gf(jl_binding_t *b, jl_module_t *mod, jl_sym_t *name); +JL_DLLEXPORT jl_value_t *jl_declare_const_gf(jl_module_t *mod, jl_sym_t *name); JL_DLLEXPORT jl_method_t *jl_method_def(jl_svec_t *argdata, jl_methtable_t *mt, jl_code_info_t *f, jl_module_t *module); JL_DLLEXPORT jl_code_info_t *jl_code_for_staged(jl_method_instance_t *linfo, size_t world, jl_code_instance_t **cache); JL_DLLEXPORT jl_code_info_t *jl_copy_code_info(jl_code_info_t *src); @@ -2051,7 +2058,7 @@ JL_DLLEXPORT jl_value_t *jl_get_binding_type(jl_module_t *m, jl_sym_t *var); // get binding for assignment JL_DLLEXPORT void jl_check_binding_currently_writable(jl_binding_t *b, jl_module_t *m, jl_sym_t *s); JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var); -JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var); +JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var, size_t new_world); JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var, int allow_import); JL_DLLEXPORT int jl_defines_or_exports_p(jl_module_t *m, jl_sym_t *var); JL_DLLEXPORT int jl_is_const(jl_module_t *m, jl_sym_t *var); diff --git a/src/julia_internal.h b/src/julia_internal.h index 5fe6cad0d096c..6d83000184880 100644 --- a/src/julia_internal.h +++ b/src/julia_internal.h @@ -876,6 +876,7 @@ STATIC_INLINE size_t module_usings_max(jl_module_t *m) JL_NOTSAFEPOINT { return m->usings.max/3; } +JL_DLLEXPORT jl_sym_t *jl_module_name(jl_module_t *m) JL_NOTSAFEPOINT; jl_value_t *jl_eval_global_var(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *e); jl_value_t *jl_interpret_opaque_closure(jl_opaque_closure_t *clos, jl_value_t **args, size_t nargs); jl_value_t *jl_interpret_toplevel_thunk(jl_module_t *m, jl_code_info_t *src); diff --git a/src/method.c b/src/method.c index 1b38a16649d8a..68542fdacabb6 100644 --- a/src/method.c +++ b/src/method.c @@ -1050,10 +1050,11 @@ JL_DLLEXPORT void jl_check_gf(jl_value_t *gf, jl_sym_t *name) jl_errorf("cannot define function %s; it already has a value", jl_symbol_name(name)); } -JL_DLLEXPORT jl_value_t *jl_declare_const_gf(jl_binding_t *b, jl_module_t *mod, jl_sym_t *name) +JL_DLLEXPORT jl_value_t *jl_declare_const_gf(jl_module_t *mod, jl_sym_t *name) { JL_LOCK(&world_counter_lock); size_t new_world = jl_atomic_load_relaxed(&jl_world_counter) + 1; + jl_binding_t *b = jl_get_binding_for_method_def(mod, name, new_world); jl_binding_partition_t *bpart = jl_get_binding_partition(b, new_world); jl_ptr_kind_union_t pku = jl_atomic_load_relaxed(&bpart->restriction); jl_value_t *gf = NULL; diff --git a/src/module.c b/src/module.c index 1b6b37e49949e..402a86dfd4aef 100644 --- a/src/module.c +++ b/src/module.c @@ -588,10 +588,10 @@ JL_DLLEXPORT jl_value_t *jl_reresolve_binding_value_seqcst(jl_binding_t *b) // get binding for adding a method // like jl_get_binding_wr, but has different error paths and messages -JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m, jl_sym_t *var) +JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m, jl_sym_t *var, size_t new_world) { jl_binding_t *b = jl_get_module_binding(m, var, 1); - jl_binding_partition_t *bpart = jl_get_binding_partition(b, jl_current_task->world_age); + jl_binding_partition_t *bpart = jl_get_binding_partition(b, new_world); jl_ptr_kind_union_t pku = jl_atomic_load_relaxed(&bpart->restriction); enum jl_partition_kind kind = decode_restriction_kind(pku); if (kind == BINDING_KIND_GLOBAL || kind == BINDING_KIND_DECLARED || jl_bkind_is_some_constant(decode_restriction_kind(pku))) @@ -601,7 +601,7 @@ JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m, jl_sym_ return b; } jl_binding_t *ownerb = b; - pku = jl_walk_binding_inplace(&ownerb, &bpart, jl_current_task->world_age); + pku = jl_walk_binding_inplace(&ownerb, &bpart, new_world); jl_value_t *f = NULL; if (jl_bkind_is_some_constant(decode_restriction_kind(pku))) f = decode_restriction_value(pku); @@ -613,7 +613,7 @@ JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m, jl_sym_ jl_module_t *from = jl_binding_dbgmodule(b, m, var); // we must have implicitly imported this with using, so call jl_binding_dbgmodule to try to get the name of the module we got this from jl_errorf("invalid method definition in %s: exported function %s.%s does not exist", - jl_symbol_name(m->name), from ? jl_symbol_name(from->name) : "", jl_symbol_name(var)); + jl_module_debug_name(m), jl_module_debug_name(from), jl_symbol_name(var)); } int istype = f && jl_is_type(f); if (!istype) { @@ -626,13 +626,25 @@ JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m, jl_sym_ // or we might want to drop this error entirely jl_module_t *from = jl_binding_dbgmodule(b, m, var); jl_errorf("invalid method definition in %s: function %s.%s must be explicitly imported to be extended", - jl_symbol_name(m->name), from ? jl_symbol_name(from->name) : "", jl_symbol_name(var)); + jl_module_debug_name(m), jl_module_debug_name(from), jl_symbol_name(var)); } } - else if (strcmp(jl_symbol_name(var), "=>") == 0 && (kind == BINDING_KIND_IMPLICIT || kind == BINDING_KIND_EXPLICIT)) { + else if (kind != BINDING_KIND_IMPORTED) { + int should_error = strcmp(jl_symbol_name(var), "=>") == 0; jl_module_t *from = jl_binding_dbgmodule(b, m, var); - jl_errorf("invalid method definition in %s: function %s.%s must be explicitly imported to be extended", - jl_symbol_name(m->name), from ? jl_symbol_name(from->name) : "", jl_symbol_name(var)); + if (should_error) + 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: Constructor for type \"%s\" was extended in `%s` without explicit qualification or import.\n" + " NOTE: Assumed \"%s\" refers to `%s.%s`. This behavior is deprecated and may differ in future versions.`\n" + " NOTE: This behavior may have differed in Julia versions prior to 1.12.\n" + " Hint: If you intended to create a new generic function of the same name, use `function %s end`.\n" + " Hint: To silence the warning, qualify `%s` as `%s.%s` or explicitly `import %s: %s`\n", + jl_symbol_name(var), jl_module_debug_name(m), + jl_symbol_name(var), jl_module_debug_name(from), jl_symbol_name(var), + jl_symbol_name(var), jl_symbol_name(var), jl_module_debug_name(from), jl_symbol_name(var), + jl_module_debug_name(from), jl_symbol_name(var)); } return ownerb; } diff --git a/stdlib/SharedArrays/src/SharedArrays.jl b/stdlib/SharedArrays/src/SharedArrays.jl index 93ce396277af7..6106bc9c3c81a 100644 --- a/stdlib/SharedArrays/src/SharedArrays.jl +++ b/stdlib/SharedArrays/src/SharedArrays.jl @@ -9,6 +9,7 @@ using Mmap, Distributed, Random import Base: length, size, elsize, ndims, IndexStyle, reshape, convert, deepcopy_internal, show, getindex, setindex!, fill!, similar, reduce, map!, copyto!, cconvert +import Base: Array import Random using Serialization using Serialization: serialize_cycle_header, serialize_type, writetag, UNDEFREF_TAG, serialize, deserialize diff --git a/test/arrayops.jl b/test/arrayops.jl index 655e14675bfb4..b2da3eac6386b 100644 --- a/test/arrayops.jl +++ b/test/arrayops.jl @@ -2948,7 +2948,7 @@ end Base.ArithmeticStyle(::Type{F21666{T}}) where {T} = T() Base.:+(x::F, y::F) where {F <: F21666} = F(x.x + y.x) -Float64(x::F21666) = Float64(x.x) +Base.Float64(x::F21666) = Float64(x.x) @testset "Exactness of cumsum # 21666" begin # test that cumsum uses more stable algorithm # for types with unknown/rounding arithmetic diff --git a/test/errorshow.jl b/test/errorshow.jl index 8f7482ce3235e..8e13d0242ae35 100644 --- a/test/errorshow.jl +++ b/test/errorshow.jl @@ -438,7 +438,7 @@ let err_str @test occursin("For element-wise subtraction, use broadcasting with dot syntax: array .- scalar", err_str) end - +import Core: String method_defs_lineno = @__LINE__() + 1 String() = throw(ErrorException("1")) (::String)() = throw(ErrorException("2")) diff --git a/test/intrinsics.jl b/test/intrinsics.jl index c3e9bb1680d48..12867908bf5a4 100644 --- a/test/intrinsics.jl +++ b/test/intrinsics.jl @@ -66,7 +66,7 @@ end # test functionality of non-power-of-2 primitive type constants primitive type Int24 24 end Int24(x::Int) = Core.Intrinsics.trunc_int(Int24, x) -Int(x::Int24) = Core.Intrinsics.zext_int(Int, x) +Base.Int(x::Int24) = Core.Intrinsics.zext_int(Int, x) let x, y, f x = Int24(Int(0x12345678)) # create something (via truncation) @test Int(0x345678) === Int(x)