8000 makeSome refactoring by pablf · Pull Request #9077 · zio/zio · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

makeSome refactoring #9077

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
Merged
merged 8 commits into from
Aug 24, 2024
Merged

makeSome refactoring #9077

merged 8 commits into from
Aug 24, 2024

Conversation

pablf
Copy link
Member
@pablf pablf commented Aug 5, 2024

Continuation of #8850

@jdegoes
Copy link
Member
jdegoes commented Aug 11, 2024

@pablf Thanks for re-opening! Can you include /fixes, etc., in the description?

@pablf
Copy link
Member Author
pablf commented Aug 11, 2024

@jdegoes the original issues are already closed, should I add something else?

kyri-petrou
kyri-petrou reviewed Aug 13, 2024
View reviewed changes
core-tests/shared/src/test/scala/zio/autowire/AutoWireSpec.scala
Comment on lines +340 to +344
def dummy(f: Any*): Boolean = {
var l: List[Any] = f.toList
l = Nil
l.isEmpty
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little bit confused, what is this testing exactly?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test intends to check that the methods before do not create warnings, so it's not useful to test only compilation. Then I got warnings about not using the methods turning into errors. I think I also tried val _ = syntax but something didn't work. It's not pretty, maybe there is a better approach...

kyri-petrou
kyri-petrou reviewed Aug 13, 2024
View reviewed changes
core/shared/src/main/scala-2/zio/internal/macros/LayerMacroUtils.scala
@@ -161,6 +161,7 @@ private[zio] trait LayerMacroUtils {
intersectionTypes
.map(_.dealias)
.filterNot(_.isAny)
.filterNot(t => typeOf[Any] <:< t)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this the same as the above .filterNot(_.isAny)?

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 am not sure. I remember that somewhere ZAny didn't get filtered and that broke the algorithm and I think it's here.

Copy link
Member Author
@pablf pablf Aug 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Testing it, isAny alone doesn't work, although just .filterNot(t => typeOf[Any] <:< t) works alone

@jdegoes jdegoes merged commit c16f288 into zio:series/2.x Aug 24, 2024
18 checks passed
@kyri-petrou
Copy link
Contributor

Hey @pablf, I just tried to update zio in a codebase at $WORK and I'm getting an exception coming from the macros when using provideSomeShared (from zio-test). Do you have any idea why this might be happening?

Exception occurred while executing macro expansion.
java.lang.Throwable: This can't happen.
	at zio.internal.macros.Graph.mkNeededKeys$$anonfun$2(Graph.scala:80)
	at zio.internal.macros.Graph.forEach$$anonfun$1(Graph.scala:189)
	at scala.collection.immutable.List.foldRight(List.scala:353)
	at zio.internal.macros.Graph.forEach(Graph.scala:194)
	at zio.internal.macros.Graph.mkNeededKeys(Graph.scala:99)
	at zio.internal.macros.Graph.buildNodes(Graph.scala:27)
	at zio.internal.macros.LayerBuilder.build(LayerBuilder.scala:93)
	at zio.internal.macros.LayerMacroUtils$.constructLayer$$anonfun$1$$anonfun$1(LayerMacroUtils.scala:110)
	at zio.internal.macros.LayerMacroUtils$.constructLayer$$anonfun$1(LayerMacroUtils.scala:110)
	at zio.internal.macros.LayerMacroUtils$.constructLayer$$anonfun$adapted$1(LayerMacroUtils.scala:110)

@pablf
Copy link
Member Author
pablf commented Aug 26, 2024 •
edited
Loading

Would you mind sharing a similar example or how the layer is being constructed? Also, which scala version is this? It seems like when retrieving a type from neededKeys using get, it doesn't find it, probably because it was added under a different name. Changing get to a comparison made using keyEquals or transforming the keys to be retrieved to be the same keys as the one in neededKeys should work.

There was a similar issue before that I wasn't able to reproduce so it seems like it was related to some configuration in how Scala treated internally the types. Relevant build details could also be useful. Does this happen in all instances of provide methods?

@guizmaii guizmaii mentioned this pull request Aug 26, 2024
Closed
@guizmaii
Copy link
Member

Reported here too: #9145 (comment)

kyri-petrou added a commit that referenced this pull request Aug 28, 2024
@kyri-petrou
Revert "makeSome refactoring (#9077)"
55defad 
This reverts commit c16f288.
@kyri-petrou kyri-petrou mentioned this pull request Aug 28, 2024
Merged
jdegoes pushed a commit that referenced this pull request Aug 28, 2024 
This reverts commit c16f288.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyri-petrou kyri-petrou kyri-petrou left review comments

@jdegoes jdegoes jdegoes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pablf @jdegoes @kyri-petrou @guizmaii
0