unicode-rs / unicode-width

Displayed width of Unicode characters and strings according to UAX#11 rules.
https://unicode-rs.github.io/unicode-width
Other
216 stars 27 forks source link

please reconsider re-publishing 0.1.13 as 0.2.0 #66

Closed decathorpe closed 2 months ago

decathorpe commented 2 months ago

Issue https://github.com/unicode-rs/unicode-width/issues/55 is striking again. It's now starting to cause real issues for us in Fedora Linux. Projects that depend on unicode-width can now be split into two categories:

Even if we did provide packages for both unicode-width <0.1.13 and >=0.1.13 (which would be unprecedented for two versions that are supposedly semver-compatible), we would need to patch all crates that currently pull in 0.1.* to have a dependency like >=0.1.0,<0.1.13 instead.

This would create situations where multiple crates in the same dependency tree pull in unicode-width at conflicting versions (either >=0.1.13 or <0.1.13), and cargo dependency resolution can't find a solution that satisfies both of these requirements. This is exactly the reason why behaviour changes are important when considering SemVer, not only API changes. So, please reconsider publishing 0.1.13+ as 0.2.x 😢

Manishearth commented 2 months ago

At the very least I need to understand the nature of how these crates "only work correctly with the a particular version of the crate". Prior reports have focused on broken tests and those tests were making incorrect assumptions.

The reason I've been holding such a hard line here is twofold:

This is exactly the reason why behaviour changes are important when considering SemVer, not only API changes.

Yes, however I think that works for behavior changes where the behavior is a core part of the crate contract. This isn't, relying on this kind of behavior not changing is a mistake. I don't wish to encourage that.

This is a common problem for Unicode stuff: people assume more about what a Unicode algorithm means than they should. People have incorrect ideas about how a concept they are used to from their daily interactions with text -- whether it be "character", "letter", "terminal width", "sorting", "equality", etc -- generalizes across all writing systems. In most cases, it doesn't cleanly generalize and you end up with a best-effort approximation that mostly gets things right but will not match a Latin-script user's intuition 100% of the time.

A pervasive assumption about unicode-width is that it provides a 100% accurate answer for terminal width: such an algorithm cannot and does not exist. For ages people kept complaining that the crate would give "incorrect" answers for emoji and we'd have to patiently explain that there is no One True Answer for terminal width obtainable from algorithms like this. We've repeatedly told users of this crate to not assume too much in the past when they've filed bugs about differences between the crate and observed terminal behavior.

We've finally improved the answers here to be a better estimation and now we have people complaining things are broken: but they should not have been relying on this being anything other than an estimation, which has been this crate's stance for ages. Crates that break typically relied upon this in a stronger way and that usually means they were broken already just not in ways they tested.

I'm quite happy to fix actual cases where the estimation provided by this crate is not ideal. It's possible that the 0.1.13 release changed things for the worse. I'm not really in favor of changing the stance that crates should expected to treat the results here as an approximation.

kwantam commented 2 months ago

@Manishearth I agree with your thinking here and completely appreciate that you've been holding the fort on this crate for many years.

In this case, however, it might worth making an exception to the policy you've articulated, just as a way of saving a huge amount of downstream work and avoiding the complexity and risk of version checks / etc. in downstream distributions.

I completely understand the issue of death-by-a-thousand-cuts and don't think this should be a regular occurrence, but on balance the amount of work to ship 0.2.0 in this instance appears to be trivial compared to what it saves.

(It goes without saying that I stand by whichever decision you make. You've been the captain of this ship for a long time :heart:)

Manishearth commented 2 months ago

Thanks!

Still, at the very least I need to understand the nature of these breakages. I don't think it makes sense to just say "0.1.13 is now 0.2.0" without understanding which changes are the actually problematic ones, and how. My concern is not the work involved in shipping 0.2, it's that we'll be in this same situation a year from now after some other similar changes.

Given that my current belief is that the ecosystem has broken assumptions here, I would rather have this opportunity be one for the ecosystem to fix its issues.

A version 0.2 could also be a way for the ecosystem to fix these assumptions, but it's not guaranteed.


Specific questions for @decathorpe:

For further context: The 0.1.13 update was not a single change: it was many small changes, and we plan to continue making such small changes. If a specific change is causing a lot of disruption, a 0.2.0 might be warranted (or maybe a backout). If it's just the volume of changes, it's hard for me to see this not happening again eventually.

decathorpe commented 2 months ago

Can you provide as many links as possible to these breakages?

This one fails with unicode-width 0.1.13 and succeeds with 0.1.12. I tried investigating the failures, and from what I can tell, it's not that the hard-coded expected results are based on wrong assumptions, but that the actual line-wrapping logic is broken by the unicode-width update.

Tests fail with unicode-width 0.1.13 and pass with 0.1.12. Failures are not caused by test code, but by non-test-code (for example, the git_delta::ansi::measure_text_width function). I can't decide whether what the function's logic does is wrong or not, but the assumptions it makes affect actual rendering of the TUI, not only comparison with expected output in tests.

Here, changes in unicode-width 0.1.13 broke rendering of the helix TUI, so not wrong assumptions in tests. helix is still pinning to unicode-width 0.1.12 to avoid breaking their UI rendering.

Here, test code needed to be adapted to work with unicode-width 0.1.13, but it is now incompatible with 0.1.12. I can't (quickly) tell if this actually broke the correct alignment of the rendering or not.

To make table rendering work correctly on the console correct with unicode-width 0.1.13, actual code changes needed to be made, and now the application is incompatible with unicode-width 0.1.12 and requires >= 0.1.13. Initial issue report about broken rendering with 0.1.13: https://github.com/lsd-rs/lsd/issues/1063

Tests fail with 0.1.13 due to completely borked table rendering. Looks like logic changes are needed here, not just updating the expected output.

---- tests::test_panic stdout ----
thread 'tests::test_panic' panicked at src/lib.rs:1322:9:
assertion `left == right` failed
  left: "+------+\n| \u{1b}[\u{1b}\0\0 |\n+------+\n"
 right: "+--+\n| \u{1b}[\u{1b}\0\0 |\n+--+\n"

These are all breakages I could find on short notice.

If they're test breakages, and if we're convinced the tests are incorrect, can Fedora disable the relevant tests?

We can skip individual tests during Fedora builds, but all issues I included above point (as far as I can tell) to logic bugs that are caused by unicode-width 0.1.13 behaviour changes. I can't in good conscience update unicode-width to v0.1.13 in Fedora Linux knowing that it will break popular applications like helix or lsd, but other libraries are now starting to adapt to changes in 0.1.13 and require 0.1.13.

In general I do think "our tests are broken because we made incorrect assumptions" is not strong enough to affect the release planning of this repo.

I agree. However, as pointed out above, his is not "just" affecting tests. There's actual bug reports from people that show that what the application is writing to the terminal during use is broken / misaligned / etc.

What are the underlying changes causing the breakages?

As far as I can tell, the change that caused most (if not all) of the problems seen here is that control characters (notably the \n character) are now considered to have width 1 instead of width 0.

Manishearth commented 2 months ago

Apologies for the delay. I've been kind of burnt out on this topic after the way it was brought up the last time on this repo, so it takes me some time to get the energy to properly think about this. I really appreciate your work going through a bunch of the failures!

I think there are a couple routes forward. It does appear to be the case that the primary issue is the newline thing. That's good; it's not a bunch of different things, but one very concrete change that we can talk about.

@Jules-Bertholet how do you feel about special casing newlines, and newlines only, to continue to return zero, given the number of breakages? We discussed this previously in https://github.com/unicode-rs/unicode-width/issues/60, but I'm curious if the number of breakages change your opinion here.

Looking at the crates that are affected by this it does seem that they are probably also suffering from other linewrapping bugs due to this assumption, as sketched out in Jules' comment in https://github.com/unicode-rs/unicode-width/issues/60#issuecomment-2179421816 . To some extent this makes me want to stick to the current design as a forcing function for folks to break incorrect Unicode assumptions

The routes I see are:

  1. Do nothing. This is still a valid option in my book: linewrapping code written using this crate really should be applying higher level breaks like \n first.
  2. Just publish a 0.2, making no other changes. This still retains the forcing function, but it makes life easier for distros since they can pin unicode-width 0.1 to 0.1.12. I am rather sympathetic to the cause of having to deal with this problem in bulk, even though at the individual level I would prefer if crates noticed this change and improved their code around this.
  3. Undo the change for \n for 0.1, publish 0.1.14, and then publish 0.2 with the \n = 1 change. Commit to not changing \n behavior in non-major versions.
  4. Undo the change for \n in 0.1, and commit to keeping \n = 0 in the 0.1 stream. Do not publish 0.2,

It is not my preference to publish a new major version of this crate, but we can do it if it would really help distros.

I'd like to have @Jules-Bertholet on board if we decide to go with the Option 3 or 4.

Jules-Bertholet commented 2 months ago

@Manishearth 3 sounds perfectly reasonable to me.

Manishearth commented 2 months ago

@Jules-Bertholet What do you think about 4? My preference is to not do a new major version, but we could.

Jules-Bertholet commented 2 months ago

@Manishearth I suppose I'd rather there be a version with '\n'=1; but it’s your crate, do as you see fit, it’s not hard to work around either way

Manishearth commented 2 months ago

@Jules-Bertholet yeah, I was basically trying to gauge the intensity of your preference here, I do care about your position here as well since you've been heavily involved in this crate.

"Not hard to work around either way" makes sense. I'm sad about losing the forcing function: a lot of these crates will have other underlying bugs due to such assumptions, but I think \n specifically is weird anyway.

Manishearth commented 2 months ago

I've made https://github.com/unicode-rs/unicode-width/pull/67, which is the first step of step 3, or just step 4 if landed on its own.

decathorpe commented 2 months ago

Thank you for reconsidering this issue! I understand that it's often frustrating to work on stuff like this.

FWIW, options 2, 3, 4 are all OK from my point of view, though without a 0.2.0 release to switch to, projects that have already adapted to changes that were in 0.1.13 will now likely either need to pin their dependency to =0.1.13 or will be broken with ^0.1 because the change was reverted in 0.1.14.

Manishearth commented 2 months ago

@decathorpe yeah, I plan to publish 0.2.0 now, those projects should just switch

(done)

decathorpe commented 2 months ago

Perfect! Thanks again. This is making my life a whole lot easier. 🙏🏼