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

don't rely on rustc bug #27

Merged
merged 2 commits into from
Aug 1, 2022
Merged

Conversation

aliemjay
Copy link
Contributor

@aliemjay aliemjay commented Jul 29, 2022

Hi,

rust-lang/rust#98835 fixes a rustc bug that makes it incorrectly accepts the following code:

async fn test<'a>() { // 'a is longer than the entire function body
    let f = |_: &'a str| {}; // expects an argument that lives as long as 'a
    f(&String::new()); // the passed reference is shorter that `'a`
}

The crater run found that figma-asset-downloader relies on this bug. I'm not sure when this change reaches stable, if ever, but it's better to not rely on such a bug. This PR tries to fix this.

If you have an opinion on this change, please let me know here or by commenting on the PR.

You can install and test the toolchain locally by running the command: rustup-toolchain-install-master cb1f7c96119295882e08b09b4bd01051669c071b

Edit: here is the error output when compiling figma-asset-downloader: https://crater-reports.s3.amazonaws.com/pr-98835/try%23cb1f7c96119295882e08b09b4bd01051669c071b/reg/figma-asset-downloader-0.9.0/log.txt

@@ -304,7 +304,10 @@ async fn get_images<'a>(

let mut futures = vec![];

let mut add_future_images = |image_ids: &'a str, format, scale| {
let mut add_future_images = |image_ids, format, scale| {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, the required change would be to only remove the lifetime annotation:

-       let mut add_future_images = |image_ids: &'a str, format, scale| {
+      let mut add_future_images = |image_ids: &str, format, scale| {

but this unfortunately exposes another rust bug that requires such a hacky solution. See rust-lang/rust#58052 rust-lang/rust#70263

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aliemjay thank you for this! could you add the explanation in the comments in line 308 or probably the link to this PR so it's clearer for people reading the code and easier to understand the motivations that led to this particular hack?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created a new issue rust-lang/rust#100002

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s great! Thank you very much!

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.

2 participants