Skip to content

Conversation

A4-Tacks
Copy link
Contributor

@A4-Tacks A4-Tacks commented Sep 7, 2025

Example

Before this PR:

struct Variant{
    field: u32
}

After this PR:

struct Variant {
    field: u32
}

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2025
@ChayimFriedman2
Copy link
Contributor

ChayimFriedman2 commented Sep 7, 2025

@A4-Tacks I appreciate your contributions to rust-analyzer (BTW, don't feel bad that your PRs go unreviewed, we're during a transition to the new trait solver and anything that's not related is a second priority).

However, about this PR and other similar to it (e.g. #20595) in particular, I have a concern. I'm talking about PRs that fix the formatting of assist-generated code. My concern is: formatting is something one rustfmt call can easily fix (I, at least, run it always, especially after triggering an assist), converting all assists to preserve it requires a lot of PRs, reviewing them is a lot of work, and making sure the formatting doesn't accidentally break is even more work. And the nail in the coffin is: after we hopefully (soon™️, after switching to the new trait solver and getting rid of mutable syntax trees and...) switch to the Roslyn trivia model, these things will just work out of the box.

To be clear: I'm not suggesting we should reject all of those PRs. I do think we need to set a policy. I don't feel strong about that either - if someone else is willing to review the PRs, and new assist PRs aren't required to maintain formatting, I'm fine with continuing to accept such PRs. I just don't want to be flooded with PRs just sitting there unreviewed, or having to maintain a high-quality and IMO redundant standard for assists.

@rust-lang/rust-analyzer thoughts?

Example
---
**Before this PR**:

```rust
struct Variant{
    field: u32
}
```

**After this PR**:

```rust
struct Variant {
    field: u32
}
```
Copy link
Member

@lnicola lnicola left a comment

Choose a reason for hiding this comment

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

In this case, at least, the changes are small and we get test updates, which will make things easier when we switch to a different flavor of syntax trees.

But in general I agree, the PR queue is getting scary.

@lnicola lnicola added this pull request to the merge queue Sep 8, 2025
Merged via the queue into rust-lang:master with commit e0aa8d4 Sep 8, 2025
15 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 8, 2025
@ShoyuVanilla
Copy link
Member

ShoyuVanilla commented Sep 8, 2025

@rust-lang/rust-analyzer thoughts?

I think any contribution should be welcomed in general, but I began to think that it would be better to avoid increasing the lines of codebase dealing with the ide assists (or other syntax tree editing features). We are planning to get rid of mutable syntax tree usages and switch to the Roslyn's trivia model as you said, and the current syntax editing infra is far from ideal because it's an intermediate thing to get rid of mutable syntax tree usages.

It would be quite burdensome to migrate existing assists and their test cases already, so adding up things would make migration harder 🤔

@A4-Tacks A4-Tacks deleted the make-record-ws branch September 9, 2025 07:40
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.

5 participants