Add explicit imports for types and fix bugs #57302
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I was playing with strengthening the semantics around #57290. However, the particular change I was trying turned out too breaking (I may try a weaker version of it). Still, there were a number of good changes found in two categories: 1. We should explicitly import types when defining constructors. This has been allowed for a long time, but we may want to consider removing it, especially given the new binding semantics which make it more confusing as in #57290. 2. There were a couple of places where I don't think it was intended for generic functions in question not to extend Base.
vtjnash
approved these changes
Feb 7, 2025
Keno
added a commit
that referenced
this pull request
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 #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
added a commit
that referenced
this pull request
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 #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
added a commit
that referenced
this pull request
Feb 8, 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 ```
Keno
added a commit
that referenced
this pull request
Feb 8, 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 ```
Keno
added a commit
that referenced
this pull request
Feb 8, 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 ```
Keno
added a commit
that referenced
this pull request
Feb 8, 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 ```
Keno
added a commit
that referenced
this pull request
Feb 10, 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 ```
Keno
added a commit
that referenced
this pull request
Feb 10, 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 ```
Keno
added a commit
that referenced
this pull request
Feb 10, 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 ```
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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was playing with strengthening the semantics around #57290. However, the particular change I was trying turned out too breaking (I may try a weaker version of it). Still, there were a number of good changes found in two categories:
We should explicitly import types when defining constructors. This has been allowed for a long time, but we may want to consider removing it, especially given the new binding semantics which make it more confusing as in No longer possible to define a function
String#57290.There were a couple of places where I don't think it was intended for generic functions in question not to extend Base.