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

Enter missing symbols generated by the MacroAnnotation expansion #18826

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

hamzaremmal
Copy link
Member

@hamzaremmal hamzaremmal commented Nov 2, 2023

Closes #18825
Closes #18806

@nicolasstucki
Copy link
Contributor

There is a failure in tests/run/quotes-add-erased. The macro migth be doing somthing that it should not and now it got caught.

@nicolasstucki
Copy link
Contributor

The failure was

 error overriding method takesErased in class TakesErased of type (erased x: Int, y: Int): Int;
  method takesErased of type (erased x: Int, y: Int): Int needs `override` modifier

I assume that we simply missed the override in the flags of the new Symbol we create in tests/run/quotes-add-erased/Macros_1.scala.

Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

Also squash and rebase the commit on main.

val isSymbolInDecls = tdef.symbol.asClass.info.decls.toList.toSet
for tree <- template.body do
if tree.symbol.owner != tdef.symbol then
report.error(em"Macro added a definition with the wrong owner - ${tree.symbol.owner} - ${tdef.symbol} in ${tree.source}")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a test that produces this error. This test should have a checkfile.

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 don't think we need this check anymore. When running the wrong-owner test, I have the following result in testlog

-- Error: tests/neg-macros/wrong-owner/Test_2.scala:7:6 ----------------------------------------------------------------
5 |@experimental
6 |@wrongOwner
7 |class Foo
|^
|Malformed tree was found while expanding macro with -Xcheck-macros.
| |The tree does not conform to the compiler's tree invariants.
| |
| |Macro was:
| |@scala.annotation.internal.SourceFile("tests/neg-macros/wrong-owner/Test_2.scala") @wrongOwner @scala.annotation.experimental class Foo()
| |
| |The macro returned:
| |@scala.annotation.internal.SourceFile("tests/neg-macros/wrong-owner/Test_2.scala") @wrongOwner @scala.annotation.experimental class Foo() {
| override def toString(): java.lang.String = "Hello from macro"
|}
| |
| |Error:
| |assertion failed: bad owner; method toString has owner class String, expected was class Foo
|owner chain = method toString, class String, package java.lang, package java, package , ctxOwners = class Foo, class Foo, package , package , package , package , package , package , package , package , package , , , , ,
| |
|stacktrace available when compiling with -Ydebug
| |
Macro added a definition with the wrong owner - class String - class Foo in tests/neg-macros/wrong-owner/Test_2.scala
2 errors found

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. The issue was that tests under the /tests/neg-macros run with the -Xcheck-macros which itself trigger the second error. We agreed with @nicolasstucki to keep both error messages in the sense that if the flag is enabled, the more developed error message will be displayed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, maybe we should add this information in the new error message. e.g "More information available under with the -Xcheck-macros"

@hamzaremmal hamzaremmal force-pushed the i18825 branch 3 times, most recently from fda646c to 27dbeb3 Compare November 8, 2023 12:52
@hamzaremmal
Copy link
Member Author

How I fixed it :

  • First, I checked if the issue is up to date.
  • When compiling the code and running it, we see that the generated code is presented in the byte code and it crashes when running.
  • After, I've checked if the generated tree is presented after the Inlining by compiling with the -Xprint:inline flag.
  • After compiling the -Xprint:inline flag, we can see that the tree is correct and not malformed. The question still remains, why is this tree skipping all the tests.
  • To answer this question, I had to change the point of view, from trees to symbols. The question remains: Are the symbol consistent ?
  • After checking, we can see that all the symbols generated by MacroAnnotations are not actually entered and therefore, the compiler is blind towards them and skips all the checks.
  • To fix, we needed to enter the new symbols and that what this PR does :-)

Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

LGTM.

@hamzaremmal could you squash your commits into one?

tests/neg-macros/wrong-owner/Test_2.scala Outdated Show resolved Hide resolved
@hamzaremmal
Copy link
Member Author

@nicolasstucki I've squashed the commits into one with all the necessary changes

@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants