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

fixing the superfluous generation of enormous amounts of temporary tuple types with UUIDs for type parameter names. #1872

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

jurgenvinju
Copy link
Member

No description provided.

…ple types with UUIDs for type parameter names.
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #1872 (976b864) into main (7c15369) will decrease coverage by 1%.
Report is 2 commits behind head on main.
The diff coverage is 100%.

@@           Coverage Diff           @@
##              main   #1872   +/-   ##
=======================================
- Coverage       49%     49%   -1%     
+ Complexity    6159    6143   -16     
=======================================
  Files          660     660           
  Lines        58668   58668           
  Branches      8543    8543           
=======================================
- Hits         28924   28902   -22     
- Misses       27556   27577   +21     
- Partials      2188    2189    +1     
Files Coverage Δ
...mpl/interpreter/matching/TypedVariablePattern.java 79% <100%> (ø)
...rascalmpl/interpreter/result/AbstractFunction.java 49% <100%> (ø)
...rg/rascalmpl/interpreter/result/NamedFunction.java 66% <100%> (ø)
...g/rascalmpl/interpreter/result/RascalFunction.java 80% <ø> (ø)
...g/rascalmpl/values/RascalFunctionValueFactory.java 56% <ø> (ø)

... and 11 files with indirect coverage changes

@jurgenvinju jurgenvinju marked this pull request as ready for review October 13, 2023 08:50
@DavyLandman
Copy link
Member

@jurgenvinju shall we add a test where curry calls itself? so to make sure the scheme works correctly even if the function is calling itself (aka the same location)

@jurgenvinju
Copy link
Member Author

Yes. I hope that works :-)

@DavyLandman
Copy link
Member

after that is done, I think this is fine to merge.

I think this would be nice to pickup before we make a new rascal release.

actualTypes.match(TypeFactory.getInstance().voidType(), renamings);

// rename all the bound type parameters
for (Entry<Type,Type> entry : renamings.entrySet()) {
Type key = entry.getKey();
renamings.put(key, TypeFactory.getInstance().parameterType(key.getName() + ":" + UUID.randomUUID().toString(), key.getBound()));
renamings.put(key, TypeFactory.getInstance().parameterType(key.getName() + ":" + uniquePrefix, key.getBound()));
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering about the performance impact of this. ISourceLocation::toString is not the fastest operation to perform. quite some string concats.

Is there an alternative that would be unique enough? perhaps the hashcode?

Copy link
Member Author

Choose a reason for hiding this comment

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

I really don't want to debug the situation where a hash collision blows this up :-) Remember this only happens for generic functions.

@jurgenvinju
Copy link
Member Author

test bool selfApplyCurry() {
    &S(&U) curry(&S(&T, &U) f, &T t) = &S (&U u) { 
      return f(t, u); 
    };

    int addition(int i, int j) = i + j;

    func = curry(curry, addition);

    assert int(int)(int) _ := func;

    func2 = func(1);

    assert int(int) _ := func2;

    return func2(1) == 2;
}

it was already one of the tests

@jurgenvinju jurgenvinju merged commit 1e66c8b into main Oct 13, 2023
7 checks passed
@jurgenvinju jurgenvinju deleted the no-more-uuid-for-type-parameters branch October 13, 2023 11:21
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