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

Builtin methods can support array-like arguments #7235

Merged

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Jul 7, 2023

Closes #7133.

In EnsoFile builtin type, we used to have builtin methods accepting array of some classes from java.io as parameters. That works only if such argument is converted with to_array. If Vector is passed, it fails.

This PR modifies the builtin method processor such that it forbids arrays of non-primitive and non-guest objects in builtin methods. And provides a proper implementation for the builtin methods in EnsoFile.

Pull Request Description

Builtin methods generator fails for a builtin method that has an array of host objects as parameter:
image

Important Notes

  • Add some useful javadoc generation to builtin methods processor.
  • Remove last Vector.to_array calls from File.enso.
  • Some quality of life improvements to the builtin method processor.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@Akirathan Akirathan linked an issue Jul 7, 2023 that may be closed by this pull request
@Akirathan Akirathan self-assigned this Jul 7, 2023
@Akirathan Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Jul 7, 2023
build.sbt Outdated
Comment on lines 961 to 969
Test / javacOptions ++= Seq(
"-s",
(Test / sourceManaged).value.getAbsolutePath
),
Compile / logManager :=
sbt.internal.util.CustomLogManager.excludeMsg(
"Could not determine source for class ",
Level.Warn
),
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes spurious errors in IDEA - the *MethodGen sources are now put into src_managed rather than on an unknown location inside target directory.

@Akirathan Akirathan marked this pull request as ready for review July 7, 2023 13:59
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Ugly, but it fixes the problem.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

@JaroslavTulach
Copy link
Member

Caused by similar design flaw as

@Akirathan Akirathan force-pushed the wip/akirathan/7133-files-input_stream-type-signatures branch from 0d2004d to 9a9ccf6 Compare July 13, 2023 16:26
Comment on lines +100 to +102
} catch (ClassCastException e) {
throw new PanicException(e, interop);
}
Copy link
Member

Choose a reason for hiding this comment

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

This should be a Type_Error also I think, not a ClassCastException wrapped in panic.

Comment on lines +90 to +93
var err = ctx.getBuiltins().error().makeUnsupportedArgumentsError(
new Object[]{arr},
"Arguments to opts should be host objects from java.io package"
);
Copy link
Member

Choose a reason for hiding this comment

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

Again, can this be a Type_Error?

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 think this is better as Unsupported_Argument_Types because it has a message field, as opposed to Type_Error that only has expected and actual types.

@Akirathan Akirathan merged commit 7264d81 into develop Jul 17, 2023
26 of 27 checks passed
@Akirathan Akirathan deleted the wip/akirathan/7133-files-input_stream-type-signatures branch July 17, 2023 07:17
return this.truffleFile.newOutputStream(opts);
public OutputStream outputStream(Object opts,
EnsoContext ctx,
@CachedLibrary(limit = "5") InteropLibrary interop) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Combination of @TruffleBoundary and @CachedLibrary - the usual error...

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, my bad. I will fix that in one of my upcoming PRs - ea95a16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File's input_stream type signatures are inconsistent
6 participants