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

Don't let the non-Enso types float around the Enso interpreter! #9584

Merged

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Mar 30, 2024

Pull Request Description

Failure described in #9462 (comment) discovered that some of our builtins are returning wrong type. Only long, double, boolean and generic TruffleObject types are expected in the Enso interpreter. Returning any other type (for example java.lang.String) leads to increased polymorphism and slowdown (or higher memory consumption) of the interpreter as the Truffle AST gets richer to handle more types.

This PR modifies the annotation processor to print a warning when a builtin's return type is suspicious. Failures are analyzed and fixed. Remaining cases where a generic java.lang.Object is being returned are annotated by @SuppressWarnings("generic-enso-builtin-type") annotation.

Checklist

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

  • All code follows the
    Scala,
    Java,
    style guides
  • All code has been tested:
    • Compiler warnings analyzed and resolved

@JaroslavTulach
Copy link
Member Author

Relates to #1239 (comment)

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 30, 2024
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Mar 30, 2024

Here are the warnings about suspicious return types:

 engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoFile.java:54:1:  Suspicious return type: java.io.OutputStream
    public OutputStream outputStream(Object opts, EnsoContext ctx) throws IOException {
                        ^
 engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoFile.java:65:1:  Suspicious return type: java.io.InputStream
    public InputStream inputStream(Object opts, EnsoContext ctx) throws IOException {
                       ^
 engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoFile.java:153:1:  Suspicious return type: java.util.Set<java.nio.file.attribute.PosixFilePermission>
    public Set<PosixFilePermission> getPosixPermissions() throws IOException {
                                    ^
 engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoFile.java:159:1:  Suspicious return type: java.lang.Object
    public Object getParent() {
                  ^
 engine/runtime/src/main/java/org/enso/interpreter/runtime/data/ManagedResource.java:63:1:  Suspicious return type: java.lang.Object
    public Object take(EnsoContext context) {
                  ^
 engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Ref.java:33:1:  Suspicious return type: java.lang.Object
    public Object getValue() {
                  ^
 engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrayLikeHelpers.java:83:1:  Suspicious return type: java.lang.Object
    public static Object vectorFromFunction(
                         ^
 engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java:46:1:  Suspicious return type: java.lang.Object
    public Object getValue() {
                  ^
 engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java:155:1:  Suspicious return type: java.lang.Object
    public static Object set(

@hubertp, @Akirathan let's discuss what do we want to do with them.

@enso-bot enso-bot bot mentioned this pull request Mar 31, 2024
4 tasks
@JaroslavTulach
Copy link
Member Author

As a result of bcf69a3 there is now AssertionError failure in tests. It should be gone once we merge #9462 - e.g. latest develop in.

@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Mar 31, 2024
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Apr 1, 2024

This is wrong, @hubertp.

results in

  Object execute(EnsoFile self) {
    EnsoContext context = EnsoContext.get(this);
    try {
      return context
          .asGuestValue(self.getCreationTime());

e.g. a TruffleObject gets wrapped in asGuestValue. The only way it can work is that asGuestValue does no wrapping of TruffleObject, but than the @Builtin.ReturningGuestObject annotation is unnecessary.

Since a98f0b7 we error on such unnecessary conversion.

@JaroslavTulach JaroslavTulach linked an issue Apr 1, 2024 that may be closed by this pull request
Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

Thanks, always nice to get rid of that hardcoded list.

};
}

static boolean isTruffleObject(ProcessingEnvironment processingEnv, TypeMirror type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@JaroslavTulach
Copy link
Member Author

Here is the current set of suspicious types. Let's:

  • not print the warning when there is @Builtin.ReturningGuestObject
  • let's allow to supress the warning in case return type is Object.

@JaroslavTulach JaroslavTulach merged commit 11e1e9e into develop Apr 2, 2024
41 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/SanityCheckBuiltinReturnTypes_8982 branch April 2, 2024 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug -compiler CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
2 participants