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

Adding getMatchFingerPrint and getConcreteMatchFingerPrint to implement a part of the compiled-runtime with more efficiency #1873

Merged
merged 20 commits into from
Nov 6, 2023

Conversation

jurgenvinju
Copy link
Member

No description provided.

@jurgenvinju
Copy link
Member Author

This is a continuation of usethesource/vallang#210

Since getMatchFingerprint also has to be implemented inside the rascal project, and another method ITree.getConcreteMatchFingerprint too, with similar sensibilities, I believe it is time we also bump the rascal project to 1.0.0. This will allow us to manage the tight integration between the compiler and the run-time with semver.

The compiler run-time integration will destabilize the rascal project, from the perspective of generated Java code by the compiler. Also, we will start removing the java JDT support and putting it in a separate project (NOT THIS PR). Such an enormous breaking change must be signalled with a major version imho.

In this PR we will add two very brittle methods to the IExternalValue implementations and to ITree.

@DavyLandman
Copy link
Member

but would that mean that we go for 1.0 now, and we'll go for 2.0 soon since we'll be dropping all kinds of things? Maybe you could line out the roadmap a bit? it's a bit unclear for me based on your comments. also fine with having a call about this.

@jurgenvinju
Copy link
Member Author

  1. there will be no 'dropping things' from the rascal project for the foreseeable future. Only if and when we can fully depend on the compiled architecture, which is a long long time from now.
  2. the interfaces IValue, IExternalValue, Type, RascalType, NonterminalType, ITree are going to receive incremental extensions as we are porting run-time features one by one from rascal-core into the rascal project. This is for efficiency reasons, but also to shape and design a stable interface between the generated code and the vallang run-time values and types. The compiler in rascal-core will be bootstrapped by its previous self, so small incremental changes are essential to making this possible.
  3. due to the previous expected interface additions, which are mostly all binary and source backward incompatible, it is good to have major version updates to signal source/binary incompatibility with previously generated code.

That's the thinking. Penny for yours?

@DavyLandman
Copy link
Member

My thoughts are that a 1.0.0 release signals a certain kind of stability, while we are heading for a somewhat unstable phase with the compiler coming up.

in semver paralance, and what we've been doing, is that for 0.x the minor field should be interpreted as a major release. so it becomes 0.major.minor. The longer I think about it, the less I see why this specific change is so different than other major releases we've done. There have been many binary incompatible releases in the past, where we've also "only" bumped the 0.x major release.

If we want to move towards a different scheme, I'm fine with discussing that, but I feel less agreement on moving to 1.0 than I feel for vallang.

and agreed, when I said "dropping things" I should have said, refactoring it to side-projects and similar plans.

@jurgenvinju
Copy link
Member Author

The difference is that these breaking changes in getMatchFingerPrint are invisible to the Java compiler and the JVM linking process. There will be no complaints. Yet the code in the rascal compiler and all the Java code generated by it depends exactly on those integer constants. So that is the reason we could bump to 1.0.0 after we introduce those functions here.

0.x.y semver signals nothing and we have not been clear to anybody about what it meant at all.

So after 1.0.0 we can continue signalling non-backward compatible external API and language changes via the major version. Any major version update in vallang will automatically lead to a major version bump here. Any API used by generated code is considered external. All other changes internal to the interpreter are not for major changes, unless the Rascal language has changed.

@jurgenvinju
Copy link
Member Author

Alternatively, we could stick with 0.x.y, but I'm afraid that we will not be able to distinguish ourselves anymore which version of the rascal runtime works with which version of the rascal compiler anymore.

@jurgenvinju
Copy link
Member Author

Another step forward without the semver is to include the entire compiler here in this project, and the run-time. That way one complex tightly coupled dependency is gone and we have simply bundled together the compiler with the run-time it depends on. It is a one-to-one tight coupling, afterall.

@jurgenvinju
Copy link
Member Author

My thoughts are that a 1.0.0 release signals a certain kind of stability, while we are heading for a somewhat unstable phase with the compiler coming up.

The 1.0.0 code is very stable; in the sense that the interpreter has worked the same for years, and most fixes have been in standard library features. So even if we would not have had this new fingerprinting feature, we could argue that the rascal project is mature now. Even if we release 2.0.0 this month, that does not make the code we use in the interpreter less stable; it means we know exactly what we are breaking and why.

Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #1873 (268e9c0) into main (1e66c8b) will increase coverage by 0%.
Report is 16 commits behind head on main.
The diff coverage is 42%.

@@           Coverage Diff           @@
##              main   #1873   +/-   ##
=======================================
  Coverage       49%     49%           
- Complexity    6147    6161   +14     
=======================================
  Files          660     661    +1     
  Lines        58668   58687   +19     
  Branches      8543    8546    +3     
=======================================
+ Hits         28893   28946   +53     
+ Misses       27577   27543   -34     
  Partials      2198    2198           
Files Coverage Δ
...rascalmpl/interpreter/result/AbstractFunction.java 49% <ø> (ø)
src/org/rascalmpl/values/functions/IFunction.java 50% <100%> (+16%) ⬆️
...c/org/rascalmpl/values/parsetrees/TreeAdapter.java 33% <ø> (ø)
src/org/rascalmpl/tasks/Transaction.java 0% <0%> (ø)
src/org/rascalmpl/values/parsetrees/ITree.java 33% <33%> (ø)
...ary/lang/rascal/matching/internal/Fingerprint.java 0% <0%> (ø)
src/org/rascalmpl/values/RascalValueFactory.java 65% <60%> (+1%) ⬆️

... and 15 files with indirect coverage changes

Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

LGTM, a few small remarks.

src/org/rascalmpl/library/vis/Charts.rsc Outdated Show resolved Hide resolved
@jurgenvinju jurgenvinju marked this pull request as ready for review November 2, 2023 12:39
@DavyLandman
Copy link
Member

Already looked good to me :) but there is still Chart.rsc changes in here? if you want them in main, then it's fine, but just checking to make sure.

@jurgenvinju jurgenvinju merged commit e13d899 into main Nov 6, 2023
6 of 7 checks passed
@jurgenvinju jurgenvinju deleted the match-fingerprints branch November 6, 2023 08:52
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.

2 participants