-
Notifications
You must be signed in to change notification settings - Fork 326
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
Add Meta.get_annotation #4049
Add Meta.get_annotation #4049
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The changes seem to be made at right places. What is still needed for this to be integrated?
engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/FunctionSchema.java
Outdated
Show resolved
Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala
Outdated
Show resolved
Hide resolved
engine/runtime/src/test/java/org/enso/compiler/EnsoCompilerTest.java
Outdated
Show resolved
Hide resolved
engine/runtime/src/test/java/org/enso/compiler/EnsoCompilerTest.java
Outdated
Show resolved
Hide resolved
|
||
"associate module annotations with method definitions" in { | ||
val ir = | ||
"""@a expr_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no, no new Scala tests, please. There is good enough infrastructure for testing IR
in ErrorCompilerTest
& co.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java tests like ErrorCompilerTest
are only checking the parsing. This test runs the compilation phases
try compiler.compile(source) | ||
finally compiler.close() | ||
} else { | ||
AstToIr.translate(source.toAst) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any need to test the old AstToIr
? The Rust based parser shall deliver good enough results. If not, such a test shall be added to EnsoCompilerTest
- that would show the difference and the difference could be eliminated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New parser produces a lot of errors with the compiler test snippets. It will take significant time to go through each case and fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be problematic soon as we want to ditch it quickly. Please file a separate ticket or (if it exists already) make sure this one is included in the tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/General/Generic ?
@@ -368,6 +385,7 @@ private List<IR> translateTypeBodyExpressionImpl(Tree exp, List<IR> appendTo) th | |||
var ir = translateTypeSignature(sig, sig.getType(), typeName); | |||
yield cons(ir, appendTo); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably standardize on that, for now this is rather dev-dependent (I have no opinion regarding the new line itself). Maybe scalafmt
can enforce that?
engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/ComplexType.scala
Outdated
Show resolved
Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/GeneralAnnotations.scala
Outdated
Show resolved
Hide resolved
try compiler.compile(source) | ||
finally compiler.close() | ||
} else { | ||
AstToIr.translate(source.toAst) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be problematic soon as we want to ditch it quickly. Please file a separate ticket or (if it exists already) make sure this one is included in the tasks.
…omplexType.scala Co-authored-by: Hubert Plociniczak <[email protected]>
Pull Request Description
Changelog:
GeneralAnnotation
IR node for@name expression
annotationsOverloadsResolution
compiler pass so that it keeps the order of module definitionsMeta.get_annotation
builtin function that returns the result of annotation expressionImportant Notes
Checklist
Please include the following checklist in your PR:
Scala,
Java,
and
Rust
style guides.
If GUI codebase was changed: Enso GUI was tested when built using BOTH
./run ide build
and./run ide watch
.