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

fix: fix glob resolution issue #3898

Open
wants to merge 1 commit into
base: 1.4.x
Choose a base branch
from

Conversation

bvanjoi
Copy link

@bvanjoi bvanjoi commented Jan 9, 2024

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

@@ -41,7 +41,7 @@ where
}

fn latest_run_migration_version(&self) -> QueryResult<Option<String>> {
use diesel::dsl::max;
Copy link
Author

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

Copy link
Member

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?

Copy link
Author

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 diesel@2.0, so I need to investigate further to determine if it's truly identical to 1.4.x.

@weiznich
Copy link
Member

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?

@bvanjoi
Copy link
Author

bvanjoi commented Jan 10, 2024

What is the impact of not issuing a patch release for this issue for diesel 1.x.

There are approximately over 200 regressions as a result of this case. For details, refer to migrations_internals-1.4.x at the following link: https://crater-reports.s3.amazonaws.com/pr-114682-1/index.html

Would it break existing applications on newer rust version

If the rust-lang/rust#114682 gets merged directly, it will lead to a build error.

or would it only result in a new warning

Initially, I intended to issue a warning regarding this matter. However, I've discovered a more complex case: max has been used as a function in __diesel_schema_migrations.select(max(version)), when it is actually intended to refer to a type max = xxx. For more details, please refer to rust-lang/rust#114682 (comment).

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 1.4.9

@weiznich
Copy link
Member

Once this PR is merged, we can advise users who encounter this regression to use 1.4.9

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.

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.

None yet

2 participants