-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: fix glob resolution issue #3898
base: 1.4.x
Are you sure you want to change the base?
Conversation
@@ -41,7 +41,7 @@ where | |||
} | |||
|
|||
fn latest_run_migration_version(&self) -> QueryResult<Option<String>> { | |||
use diesel::dsl::max; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diesel::dsl
has re-exported an ambiguous max, one exported from dsl::helper_types::*
and another from dsl::expression::dsl
, see: https://github.com/diesel-rs/diesel/blob/1.4.x/diesel/src/lib.rs#L220-L224
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern continues to exist in diesel 2.0. See #3745 for some discussion around that. As it is hard to completely remove this bug without doing a breaking change (and therefore a major version bump) what's the impact on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern continues to exist in diesel 2.0
The regression test report doesn't indicate any regression in [email protected]
, so I need to investigate further to determine if it's truly identical to 1.4.x.
Thanks for opening this PR. What is the impact of not issuing a patch release for this issue for diesel 1.x. Would it break existing applications on newer rust version or would it only result in a new warning? |
There are approximately over 200 regressions as a result of this case. For details, refer to
If the rust-lang/rust#114682 gets merged directly, it will lead to a build error.
Initially, I intended to issue a warning regarding this matter. However, I've discovered a more complex case: Therefore, resolving this issue simply as a warning won't truly address this regression. It requires additional work, such as ignoring the type namespace if there is ambiguity in the binding defined at the extern crate. However, implementing this change doesn't seem worthwhile as the problem can be resolved by submitting this PR. Once this PR is merged, we can advise users who encounter this regression to use |
The main reason why I asked is because I consider diesel 1.4.x as not actively supported anymore. I fear that issuing another update will raise expectations from users that did not update yet that we will continue to provide fixes and other stuff for this old version. That's nothing I'm able to do on the side. For that reason I would prefer not having to issue an update that fixes an breaking change done by rustc. |
Hi, we have recently made some changes related to glob name resolution in rustc and identified some regressions in this case. This PR aims to address these issues. You can find more background information at: rust-lang/rust#114682