search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.23k stars 1.58k forks source link

Sort members isn't stable #44759

Open bwilkerson opened 3 years ago

bwilkerson commented 3 years ago

The 'sort members' operation should not make changes except when necessary because of moving a member to the correct location, but it appears to not be stable when some members have a blank line between them and others don't. See https://dart-review.googlesource.com/c/sdk/+/180721/2/pkg/analyzer/lib/src/dart/micro/analysis_context.dart#107 for an example.

bwilkerson commented 3 years ago

@scheglov

incendial commented 1 year ago

@bwilkerson @srawlins is this issue still valid?

I'd like to share the implementation I have for DCM where this problem is solved. Can create a CL. WDYT?

bwilkerson commented 1 year ago

To the best of my knowledge this issue hasn't been addressed.

Contributions are always welcome.

incendial commented 1 year ago

Done: https://dart-review.googlesource.com/c/sdk/+/281441

incendial commented 1 year ago

@srawlins curious why the failed test is the one I've not touched at all. You're waiting for me to fix it before the review?

Screenshot 2023-02-08 at 13 29 29
srawlins commented 1 year ago

Yes please. I imagine this test failed because you edited the sorting code.

incendial commented 1 year ago

Yes please. I imagine this test failed because you edited the sorting code.

Ah, didn't think of that. Fixed (was a bug, actually).

incendial commented 1 year ago

@srawlins the more I think about this CL, the more I wonder - maybe instead of the hardcoded ' ' it should be calculated (if the user has tabs instead of spaces)?

incendial commented 1 year ago

@srawlins @bwilkerson any update on the review?

srawlins commented 1 year ago

No update, it could take a while, apologies.

incendial commented 1 year ago

@srawlins should I keep an eye on this and ping from time to time or you'll get back when you have time?

srawlins commented 1 year ago

Sorry for the delayed response!! I'm just kicked off CI again on your PR and I'd love to merge it soon.

incendial commented 1 year ago

@srawlins I see that checks failed because of the analysis, but this looks fast to fix. Will there be more review comments, so I can address them and the failures at once? Or fixing the warnings is the only step left?

srawlins commented 1 year ago

A fair question! As it has not been '+1''d yet. I reviewed it and I think I will approve ASAP. Let me take one more look.

FMorschel commented 1 week ago

I'm not sure this is still happening. I could not replicate it. Can anyone confirm this? Maybe I'm doing something wrong or didn't understand how to trigger the problem.