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

Remove workarounds for Rust bugs in Yoke, DataPayload #1061

Closed
sffc opened this issue Sep 16, 2021 · 10 comments · Fixed by #2185
Closed

Remove workarounds for Rust bugs in Yoke, DataPayload #1061

sffc opened this issue Sep 16, 2021 · 10 comments · Fixed by #2185
Assignees
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-small Size: One afternoon (small bug fix or enhancement) T-docs-tests Type: Code change outside core library

Comments

@sffc
Copy link
Member

sffc commented Sep 16, 2021

There are docs tests in the yoke and icu_provider crate that will begin compiling in Rust 1.56 or 1.57, due to fixes to upstream bugs rust-lang/rust#86703 (fixed in 1.56) and rust-lang/rust#88446 (fixed in 1.57). This issue is to track steps to update to these Rust versions once stabilized and at the same time update our docs tests.

As part of this bug, also remove the obsolete with_capture variants of the functions, and fix alignment between Yoke and DataPayload.

CC @Manishearth

@sffc sffc added T-docs-tests Type: Code change outside core library C-data-infra Component: provider, datagen, fallback, adapters S-tiny Size: Less than an hour (trivial fixes) labels Sep 16, 2021
@sffc sffc self-assigned this Sep 22, 2021
@sffc sffc added this to the 2021 Q3 0.4 Sprint C milestone Sep 22, 2021
@sffc
Copy link
Member Author

sffc commented Oct 1, 2021

This is partially fixed by #1090 and #1135, but we are now blocked by the following bugs:

This new state is reflected in #1134. Moving to the 0.5 milestone to revisit.

@sffc sffc modified the milestones: 2021 Q3 0.4 Sprint C, ICU4X 0.5 Oct 1, 2021
@sffc sffc changed the title Update ICU4X docs tests to Rust 1.56 and 1.57 Remove workarounds for Rust bugs in Yoke, DataPayload Oct 1, 2021
@sffc sffc added S-small Size: One afternoon (small bug fix or enhancement) and removed S-tiny Size: Less than an hour (trivial fixes) labels Oct 3, 2021
@Manishearth
Copy link
Member

Manishearth commented Nov 16, 2021

There may be a workaround that means we can get rid of _with_capture: rust-lang/rust#89436 (comment) (but it requires more annotations)

@sffc sffc modified the milestones: ICU4X 0.5, 2022 Q1 0.6 Sprint A Jan 27, 2022
@sffc
Copy link
Member Author

sffc commented Jan 27, 2022

I'll take another look now that 1.58 is stable.

@sffc
Copy link
Member Author

sffc commented Mar 25, 2022

OK, I was able to enable all of the docs tests that we had marked as "to be enabled" after 1.57.

It appears that we still need YokeTraitHack and attach_to_cart_badly. However, I'm not sure how to test whether we still need project_with_capture; it takes a fn argument, and I assume we'd need to test against a function signature that takes a function trait argument.

Many of the call sites reference rust-lang/rust#84937, which is closed, but the code still doesn't compile (perhaps for different reasons than the issue was originally raised for). I saw this comment, but I'm not immediately sure how to apply it.

rust-lang/rust#89436 (comment)

I'm going to reassign this issue to Manish to take another look at some point before we stamp 1.0.

@robertbastian
Copy link
Member

robertbastian commented May 22, 2022

1.61 seems to make attach_to_cart_badly unnecessary, its docs test starts compiling.

@Manishearth
Copy link
Member

Let's keep it around but start migrating

@sapriyag sapriyag added the discuss-priority Discuss at the next ICU4X meeting label May 25, 2022
@sffc
Copy link
Member Author

sffc commented May 26, 2022

Discuss: Should we keep the badly methods or not?

@sffc
Copy link
Member Author

sffc commented Jun 3, 2022

Discussion:

  • @Manishearth - For _badly, we should keep them until at least 6 bug-free months have passed.
  • @sffc - How about _with_capture?
  • @robertbastian - Can we upstream our tests to the Rust compiler?
  • @Manishearth - We should try to upload minimal working examples, which might be tricky, but yes, I would support that.
  • @Manishearth - We should add the buggy version of map_project so that we can test it.

AIs:

  • @Manishearth to file an issue to track adding the tests.
  • @Manishearth to change map_project and map_project_cloned to use a closure.

@sffc sffc removed the discuss-priority Discuss at the next ICU4X meeting label Jun 3, 2022
@sffc
Copy link
Member Author

sffc commented Jun 3, 2022

This is 1.0 because it affects DataPayload APIs.

@Manishearth
Copy link
Member

Upstream for map_project: rust-lang/rust#99257

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-small Size: One afternoon (small bug fix or enhancement) T-docs-tests Type: Code change outside core library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants