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.

Sign up for GitHub

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

Jump to bottom

Don't add module's builtins to the scope of a builtin type #3791

Merged
merged 19 commits into from
Nov 16, 2022
Merged

Don't add module's builtins to the scope of a builtin type #3791

merged 19 commits into from
Nov 16, 2022

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Oct 12, 2022 •
edited
Loading

Pull Request Description

It appears that we were always adding builtin methods to the scope of the module and the builtin type that shared the same name.
This resulted in some methods being accidentally available even though they shouldn't.

This change treats differently builtins of types and modules and introduces auto-registration feature for builtins.
By default all builtin methods are registered with a type, unless explicitly defined in the annotation property.
Builtin methods that are auto-registered do not have to be explicitly defined and are registered with the underlying type.
Registration correctly infers the right type, depending whether we deal with static or instance methods.

Builtin methods that are not auto-registered have to be explicitly defined always. Modules' builtin methods are the prime example.

Important Notes

Builtins now carry information whether they are static or not (inferred from the lack of self parameter).
They also carry a autoRegister property to determine if a builtin method should be automatically registered with the type.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@hubertp hubertp requested a review from kustosz October 12, 2022 16:21
@hubertp hubertp force-pushed the wip/hubert/builtins-creap-into-type-scope-183403598 branch from fde582a to 01b2b08 Compare October 14, 2022 10:22
@hubertp hubertp changed the title Don't add builtins to the scope of builtin type Don't add static builtins to the scope of builtin type Oct 14, 2022
@hubertp hubertp force-pushed the wip/hubert/builtins-creap-into-type-scope-183403598 branch 2 times, most recently from 07f1f90 to 436f7ce Compare October 27, 2022 21:03
@hubertp hubertp marked this pull request as ready for review October 27, 2022 21:36
radeusgd
radeusgd approved these changes Oct 28, 2022
View reviewed changes
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good, just a few suggestions/questions.

distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso Show resolved Hide resolved
...g/enso/interpreter/node/expression/builtin/immutable/FromPolyglotArrayBuiltinVectorNode.java Outdated Show resolved Hide resolved
...untime/src/main/java/org/enso/interpreter/node/expression/builtin/special/RunThreadNode.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Array.java Outdated Show resolved Hide resolved
engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/PolyglotTest.scala Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/phase/ModuleBuiltins.scala Outdated Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala Outdated Show resolved Hide resolved
lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/MethodProcessor.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Builtins.java Outdated Show resolved Hide resolved
JaroslavTulach
JaroslavTulach approved these changes Oct 31, 2022
View reviewed changes
...-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/BuiltinTypesTest.scala Show resolved Hide resolved
.../java/org/enso/interpreter/node/expression/builtin/immutable/FromArrayBuiltinVectorNode.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/io/PrintErrNode.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/BuiltinFunction.java
import org.enso.interpreter.runtime.callable.function.Function;

/** BuiltinFunction encapsulates information about a builtin runtime function and its metadata. */
public class BuiltinFunction {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't BuiltinFunction rather be a record than plain class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, indeed. Except that frgaal appears to be causing problems when I make that change, as noted in the comment one line below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, still does it

[error] /home/hubert/tmp/enso/engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/BuiltinFunction.java:7:15: not found: type java
[error] public record BuiltinFunction(Function fun, boolean isStatic, Owner owner) {
[error]               ^
[error] one error found

this is a weird one.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a simple reproducer that wouldn't involve sbt? Then we can report a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems that might be a known caveat - "record classes" section mentions some problems if the target is lower than 16.

@hubertp hubertp force-pushed the wip/hubert/builtins-creap-into-type-scope-183403598 branch from 983efc8 to 16157ae Compare October 31, 2022 10:56
@hubertp hubertp force-pushed the wip/hubert/builtins-creap-into-type-scope-183403598 branch from c690ad3 to b3c8eed Compare November 15, 2022 15:54
hubertp and others added 16 commits November 15, 2022 17:03
@hubertp
Don't add builtins to the scope of builtin type
774bdf0 
It appears that we were always adding builtin methods to the scope of
the module and the builtin type that shared the same name.

This resulted in some methods being accidentally available even though
they shouldn't.
@hubertp
Split registration of builtins
71d6beb 
During Builtins initialization we only register non-static builtins.
That way one does not have to import stdlib to have basic operations on
Any or Number.
Similarly during IRtoTruffle for only consider static builtins, thus
avoiding a situation where we would try to register them twice.

This required changing our annotations processor so that it is aware if
a builtin is a static or not, based on the lack or presence of self
parameter, respectively.

More importantly this approach allows us to avoid hacky solution in
types which caused adding module methods to types' scope. And we don't
do that late in the interpreter.
@hubertp
Cleanup
5334add
@hubertp
more cleanup
de00c57
@hubertp
Minor adaptations to latest changes
fedb0a4
@hubertp
nits
a07b785
@hubertp
Carry information about the owner of the builtin
cd91c2c 
Builtin statics are registered differently depending on whether their
owner would be a module or a type. The latter can be registered early on
because we already have type presents, while the former will be
registered during codegen.

This distinction allows us to treat all builtins the same. There is no
need to make a stub definition of a builtin method - they will all be
automatically registered, and the fact if they are static or not is now
irrelevant.
@hubertp
Inject modules' builtin methods defs before passes
2ab216e 
Module builtin methods declarations had to be implicitly added at an
early stage so that BindingAnalysis (and later passes) can make use of
that information.
This change adds implicit method definitions for all module builtins
unless they have been provided explicitly.

With this change one does not have to provide explicit builtin
declarations ever, irrespective of whether the owner is a module or a
type. Removed some stub definitions to illustrate the usage.
@hubertp
rebase tweaks
fce1dad
@hubertp
changelog
9909f9b
@hubertp
fmt
9ae2d6d
@hubertp
Remove now obsolete declarations
24224cc
@hubertp
nits
5882049
@hubertp
PR review
f09af2a
@hubertp @radeusgd
Apply suggestions from code review
7387381 
Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>
@hubertp
PR review + formatting
5ea2d6f
@hubertp
Use autoRegister rather than owner for builtins
4a0d272 
Builtins defined on modules are no longer auto-registered by default.
This was suggested to be a more intuitive solution that avoids magic and
potential name conflicts for modules using the same name.
@hubertp
Missing builtin declaration in micro-distribution
3c9c0b0
@hubertp hubertp force-pushed the wip/hubert/builtins-creap-into-type-scope-183403598 branch from b3c8eed to 3c9c0b0 Compare November 15, 2022 16:36
@hubertp hubertp changed the title Don't add static builtins to the scope of builtin type Don't add module builtins to the scope of builtin type Nov 16, 2022
@hubertp hubertp changed the title Don't add module builtins to the scope of builtin type Don't add module's builtins to the scope of a builtin type Nov 16, 2022
@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Nov 16, 2022
@mergify mergify bot merged commit 7b0759f into develop Nov 16, 2022
@mergify mergify bot deleted the wip/hubert/builtins-creap-into-type-scope-183403598 branch November 16, 2022 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radeusgd radeusgd radeusgd approved these changes

@JaroslavTulach JaroslavTulach JaroslavTulach approved these changes

@kustosz kustosz Awaiting requested review from kustosz

@jdunkerley jdunkerley Awaiting requested review from jdunkerley jdunkerley is a code owner

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hubertp @radeusgd @JaroslavTulach