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

Red knot: Add file watching and cancellation #11127

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Apr 24, 2024

Summary

Okay, I must admit this PR does more than what the title says.

The changes not covered by the title are:

  1. It adds a new lint_syntax method that performs two checks. It mainly serves to see if file changes are correctly picked up. The implemented checks are:
  2. Whether any line contains more than 88 characters (yeah, it should be the line width, but I didn't bother for the prototype)
  3. Whether any string literal uses single quotes instead of double quotes (again, a very dumb implementation. It even flags strings that contain double quotes, which normally would be considered okay)
  4. It changes Source to use SourceKind instead of always an Arc<str>. This is necessary to support IPython notebooks. There's absolutely no good reason why this is part of this PR and I'm happy to split it out if people find that useful. I think it doesn't matter much.

The main change of the PR is that red-knot now accepts an "entry path" and runs the analysis in watch mode. This is interesting because it tackles a few challenges:

  • How to wait for file changes or until the user presses Ctrl+C without using a spin loop. That's not conceptually difficult, but it's something I've never done before, so it took me a few approaches to get it right.
  • How can we ensure that we get a mutable reference to program after all scheduled analysis have been completed
  • How to cancel pending analysis.

I'm undecided if the ultimate goal should be that this code is isolated and reused across the LSP, CLI watch mode and regular CLI. It would certainly be nice, but I could also see this be part of the "Host" specific code, and each host should have as much flexibility as possible to implement the most efficient scheduling for its needs.

Future Improvements:

  • We should ignore changes to files that are ignored in the settings and isn't a dependency of Program
  • The current error handling is mostly unwrapping. We should do better.
  • File watching...: Correctly handle folders, new files, cases where a user deletes a file and creates a folder with the same name etc.
  • The invalidation logic requires more love, but I don't think it's important to fully implement it right now because the PR proves the important point, that we can get a mutable Program (db)

Test plan

I ran the CLI locally, then made changes to the test file by adding a new single quoted string and I then verified that the console showed the new diagnostic.

@MichaReiser MichaReiser changed the base branch from main to red-knot April 24, 2024 13:23
@MichaReiser MichaReiser added the internal An internal refactor or improvement label Apr 24, 2024
@MichaReiser MichaReiser marked this pull request as ready for review April 24, 2024 13:40
@MichaReiser
Copy link
Member Author

CC: @BurntSushi for the case I do anything horrible inside of main.rs

Copy link
Contributor

github-actions bot commented Apr 24, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser
Copy link
Member Author

MichaReiser commented Apr 24, 2024

Okay, I think what I want to do tomorrow is:

  • Figure out if we can move the orchestration thread out of the main loop. I suspect that this should be straightforward by adding one more channel 😆
  • Add a program.check(context) and program.check_file(id, context) functions and move the logic for checking a program there. The context would expose a method to schedule a dependency so that it's up to the host to decide if that should run on the same thread (queuing) or on a thread pool.

Edit: I think I should also be able to move the Arc<AtomicUsize> counting the pending analysis into the orchestrator by adding a new StartAnalysis message (it means some more messaging but it allows isolating the logic and I could even use a regular usize)

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Very cool! Sorry I didn't see this / review it sooner so you could have landed it today.

I didn't carefully follow every detail in main.rs; hoping that as you mentioned some of that will get broken up and be a little easier to follow.

crates/red_knot/src/lint.rs Outdated Show resolved Hide resolved
crates/red_knot/src/lint.rs Show resolved Hide resolved
crates/red_knot/src/program.rs Outdated Show resolved Hide resolved
crates/red_knot/src/main.rs Outdated Show resolved Hide resolved
crates/red_knot/src/main.rs Show resolved Hide resolved
crates/red_knot/src/watch.rs Show resolved Hide resolved
@MichaReiser MichaReiser force-pushed the red-knot-file-watching branch from b8d42b7 to 9f29299 Compare April 25, 2024 07:33
@MichaReiser
Copy link
Member Author

Very cool! Sorry I didn't see this / review it sooner so you could have landed it today.

All good. I'm not in a rush. Having the review next morning is still a perfectly quick review and I should have tagged you as reviewer! Thank you

@MichaReiser MichaReiser merged commit 608a182 into red-knot Apr 25, 2024
18 checks passed
@MichaReiser MichaReiser deleted the red-knot-file-watching branch April 25, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants