8000 fix assertTrue on new instance creation by goshacodes · Pull Request #9832 · zio/zio · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix assertTrue on new instance creation #9832

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

Merged
merged 2 commits into from
May 2, 2025

Conversation

goshacodes
Copy link
Contributor
@goshacodes goshacodes commented Apr 30, 2025

This PR improves support for creation of new objects in assertTrue macro in case of overloaded methods

There were several problems:

  1. For anonymous classes the tree is Block(List(cls: ClassDef), term) and it has no symbol. So we catch it and take correct symbol.
  2. For inlined anonymous classes the tree is Typed(Block(List(cls: ClassDef), term), _) and it has no symbol too. Solution is same, but also I used underlyingArgument to remove Inlined nodes.

Also:

  1. Added error message in case something other has no symbol, so it will be easier to understand what tree is not working.
  2. Changed signature of getFieldOrMethod to remove some boilerplate. Now it accepts TypeRepr and takes its typeSymbol. Note that it affects one of cases where lhs.symbol was used and now it is lhs.tpe.typeSymbol. IMO using lhs.symbol is not correct and I'm sure about it - if you think I'm wrong, we can discuss it.
  3. added scala-3 directory as sources for scala 3 to make some specific test cases, e.g. with inline.

#9683

@goshacodes goshacodes force-pushed the fix_assert branch 2 times, most recently from bfa29b8 to 9a60206 Compare April 30, 2025 06:54
Copy link
Collaborator
@hearnadam hearnadam left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

object SmartAssertionScala3Spec extends ZIOBaseSpec {

override def spec =
suite("SmartAssertionScala3Spec")(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this only contain Scala 3 specific tests? Do we need the overlap with SmartAssertionSpec?

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, only specific to scala 3 test cases.

  1. trait with parameters
  2. inlines
  3. assertTrue with anonymous classes not compiled with scala 2 and it looked like scala 2 problem itself

If you have any suggestions on improving it - I can do it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I didn't think about traits with parameters not being available on Scala 2. Thanks for the clarification.

.find(f => f.name == name)
.orElse(s.methodMember(name).filter(_.declarations.nonEmpty).headOption)
.getOrElse(
throw new Error(s"Could not resolve $name on ${owner.show(using Printer.TreeStructure)}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to use reportErrorAndAbort? Is it also possible to test these failure cases, or do we expect them to be unreachable (for now)?

Copy link
Contributor Author
@goshacodes goshacodes May 1, 2025

Choose a reason for hiding this comment

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

Sure, I'll change it.

This should be unreachable if chosen TypeRepr if correct, because we know here that was selected a val or a method without arguments, so it should exist. It can be reachable only in case of incorrect implementation.

@kyri-petrou kyri-petrou merged commit 9b77fb3 into zio:series/2.x May 2, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0