rascal icon indicating copy to clipboard operation
rascal copied to clipboard

Properly implement the watch feature

Open DavyLandman opened this issue 5 months ago • 1 comments

This PR introduces:

  • a new implementation of the watch feature that on OSX used the native APIs that avoid the current JDK polling
  • fixes a range of bugs found in our emulation of the watches for resolvers that did not support it nativly
  • allows resolvers to support recursive watches
  • fix bugs in implementation that would simulate the recursive watches (for example, it would not register a watch in a new directory)

This replaces the #1659 PR.

Some design issues I'm considering:

  • [ ] the rascal api always reports if a change was a directory or a file, but you can't know that, since it's either a deleted event, or a created event that is processed after the file has been deleted.

DavyLandman avatar Jun 18 '25 09:06 DavyLandman

Codecov Report

Attention: Patch coverage is 57.89474% with 128 lines in your changes missing coverage. Please review.

Project coverage is 47%. Comparing base (3f6314a) to head (6217f6f). Report is 42 commits behind head on main.

Files with missing lines Patch % Lines
...rascalmpl/uri/watch/SimulatedRecursiveWatcher.java 0% 62 Missing :warning:
src/org/rascalmpl/uri/watch/WatchRegistry.java 72% 25 Missing and 19 partials :warning:
src/org/rascalmpl/uri/file/FileURIResolver.java 82% 2 Missing and 6 partials :warning:
src/org/rascalmpl/uri/URIResolverRegistry.java 60% 5 Missing and 1 partial :warning:
src/org/rascalmpl/ideservices/IDEServices.java 0% 3 Missing :warning:
...org/rascalmpl/library/util/IDEServicesLibrary.java 0% 3 Missing :warning:
src/org/rascalmpl/interpreter/Evaluator.java 0% 2 Missing :warning:
Additional details and impacted files
@@           Coverage Diff            @@
##              main   #2305    +/-   ##
========================================
  Coverage       47%     47%            
- Complexity    6442    6472    +30     
========================================
  Files          764     766     +2     
  Lines        63286   63422   +136     
  Branches      9454    9462     +8     
========================================
+ Hits         30126   30228   +102     
- Misses       30888   30924    +36     
+ Partials      2272    2270     -2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jun 18 '25 09:06 codecov[bot]

For reference here is the data definitions for changed events in IO.rsc

data LocationChangeEvent
    = changeEvent(loc src, LocationChangeType changeType, LocationType \type);

data LocationChangeType
    = created() 
    | deleted() 
    | modified();

data LocationType
    = file() 
    | directory();

and here we have the DocumentEdit data definitions from analysis::diff:edits:

data DocumentEdit
    = removed(loc file)
    | created(loc file)
    | renamed(loc from, loc to)
    | changed(loc file, list[TextEdit] edits)
    ;

So there's seem an opportunity for sharing,

jurgenvinju avatar Jun 24 '25 12:06 jurgenvinju

My biggest problem is the LocationType \type this cannot be reliable calculated, for example in case of a deleted file or directory.

DavyLandman avatar Jun 24 '25 15:06 DavyLandman

So let's get rid of that distinction between files and directories. It was only there because of the original Java API.

jurgenvinju avatar Jun 24 '25 16:06 jurgenvinju

And then it becomes eerily close to DocumentEdit so it is perhaps thinkable that we unify these two concepts? We don't have exact changes, so we could add a constructor for changed without the list[TextEdit]? And that would be that, or we could make the edits an optional keyword parameter.

This kind of reconcilliations keep our libraries small.

jurgenvinju avatar Jun 24 '25 16:06 jurgenvinju

So let's get rid of that distinction between files and directories. It was only there because of the original Java API.

I was curious if we missed something during the design of java-watch but the original code had the same issue: it would call isDirectory on the URIResolverRegistry for every event. So I'm quite sure this was already an issue in the current design.

And then it becomes eerily close to DocumentEdit so it is perhaps thinkable that we unify these two concepts?

I like reducing the surface of our library, but the changes can also be about added or removed directories, and then a DocumentEdit seems a bit weird?

DavyLandman avatar Jun 25 '25 07:06 DavyLandman

Let's come up with a better name for DocumentEdit; it wasn't so good in the first place.

jurgenvinju avatar Jun 26 '25 13:06 jurgenvinju