Skip to content

Commit

Permalink
Add a warning for auto-import of types
Browse files Browse the repository at this point in the history
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
```
  • Loading branch information
Keno committed Feb 7, 2025
1 parent 778e079 commit bab77c4
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 33 deletions.
25 changes: 4 additions & 21 deletions src/codegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1019,7 +1019,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,
};
Expand Down Expand Up @@ -6868,8 +6868,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;
Expand All @@ -6878,26 +6876,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);
Expand Down
3 changes: 1 addition & 2 deletions src/interpreter.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/julia.h
Original file line number Diff line number Diff line change
Expand Up @@ -1901,7 +1901,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);
Expand Down Expand Up @@ -2051,7 +2051,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);
Expand Down
3 changes: 2 additions & 1 deletion src/method.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
24 changes: 18 additions & 6 deletions src/module.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Expand All @@ -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);
Expand Down Expand Up @@ -629,10 +629,22 @@ JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m, jl_sym_
jl_symbol_name(m->name), from ? jl_symbol_name(from->name) : "<null>", 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) : "<null>", 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_symbol_name(m->name), from ? jl_symbol_name(from->name) : "<null>", jl_symbol_name(var));
else
jl_printf(JL_STDERR, "WARNING: Type %s.%s was auto-`import`ed in `%s`.\n"
"NOTE: This behavior is deprecated and may error in future Julia versions.\n"
"NOTE: This behavior may have differed in Julia versions prior to 1.12 depending on binding resolution."
"Hint: To retain the current beavior, add an explicit `import %s: %s`\n"

Check warning on line 642 in src/module.c

View workflow job for this annotation

GitHub Actions / Check for new typos

perhaps "beavior" should be "behavior".
"Hint: To create a new generic function of the same name use `function %s end`.\n",
from ? jl_symbol_name(from->name) : "<null>", jl_symbol_name(var),
jl_symbol_name(m->name)
from ? jl_symbol_name(from->name) : "<null>", jl_symbol_name(var),
jl_symbol_name(var));
}
return ownerb;
}
Expand Down
2 changes: 1 addition & 1 deletion test/errorshow.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down

0 comments on commit bab77c4

Please sign in to comment.