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

Tweak macro to allow more than 22 inputs #1623

Merged
merged 6 commits into from
Dec 16, 2021
Merged

Tweak macro to allow more than 22 inputs #1623

merged 6 commits into from
Dec 16, 2021

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Dec 15, 2021

Fixes #910

Basically we avoid using the statically typed zipMap methods which cap out at 22, and instead use an untyped zipMapLong method that works with IndexedSeq[T[Any]] and IndexedSeq[Any]s. This could conceivably result in runtime errors if the macro has bugs, but assuming the macro is correct then it's as safe for the user as the existing macro is

By removing the old implementation, we are able to cut out a considerable amount of complexity from the codebase while still maintaining the existing semantics. Performance is probably better after, since we're just removing a layer of indirection: previously we ended up going through this dynamic Seq[Any] anyway, we just had a statically-typed facade wrapping it

Tested with a unit test, also tested manually in the scratch folder on the following build:

def t1 = T{ 1 }
def t2 = T{ 2 }
def t3 = T{ 3 }
def t4 = T{ 4 }
def t5 = T{ 5 }
def t6 = T{ 6 }
def t7 = T{ 7 }
def t8 = T{ 8 }
def t9 = T{ 9 }
def t10 = T{ 10 }
def t11 = T{ 11 }
def t12 = T{ 12 }
def t13 = T{ 13 }
def t14 = T{ 14 }
def t15 = T{ 15 }
def t16 = T{ 16 }
def t17 = T{ 17 }
def t18 = T{ 18 }
def t19 = T{ 19 }
def t20 = T{ 20 }
def t21 = T{ 21 }
def t22 = T{ 22 }
def t23 = T{ 23 }

def sum = T{
  t1() + t2() + t3() + t4() + t5() + t6() + t7() + t8() + t9() + t10() + t11() + t12() +
  t13() + t14() + t15() + t16() + t17() + t18() + t19() + t20() + t21() + t22() + t23()
}

This passes on this PR, fails on master.

@lefou
Copy link
Member

lefou commented Dec 15, 2021

If there is no evidence, that the typed zipMap has great benefits, we should remove it in favor of the dynamic approach. This would make the codebase simpler. Also, it would contribute to our confidence that the macros are as robust as the static version, when we use it in all situations. We could keep the static version in some test library, so that we could race both implementations or something like that.

I once tried to deduplicate the inputs (to avoid using multiple params for the same input), but failed, probably because the amount of code and locations where we generate code for the static version and the way all this is used in macros. So, Maybe this is an opportunity to make this part of code and code generation a bit more comprehensible. (I then fixed the duplication of inputs at least on the surface, e.g. in the output of inspect).

@lefou
Copy link
Member

lefou commented Dec 15, 2021

I think your manual test case should also fail on current main, if you are using the same input more than 22 time.

@lihaoyi
Copy link
Member Author

lihaoyi commented Dec 16, 2021

@lefou I went further with the "delete old way of doing things" approach. It really does simplify the codebase a lot!

I also got rid of all the nested .zips by defining a simple top-level Task.TraverseCtx class that performs this in one graph node (had to tweak some tests to account for the smaller number of nodes involved in traversals due to it)

@lihaoyi lihaoyi requested review from lefou and lolgab December 16, 2021 01:42
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

This is really a nice cleanup. No generated classes anymore. Very nice! Overall looks good to me, although I have to admit, I wouldn't notice any issue in the macros just from looking at it in the browser. I do macro stuff too seldom. But the passed test suite makes me rather confident.

@lihaoyi lihaoyi merged commit bb37e70 into main Dec 16, 2021
@lefou lefou deleted the 22plus branch December 16, 2021 07:40
@lefou lefou added this to the 0.10.0-M5 milestone Dec 16, 2021
@lihaoyi
Copy link
Member Author

lihaoyi commented Dec 16, 2021

Yeah the macro code is pretty gnarly, it was a lot of trial and error. On the other hand our test suite is pretty comprehensive, so hopefully if it works there it'll work everywhere

pbuszka pushed a commit to pbuszka/mill that referenced this pull request Dec 26, 2021
Fixes com-lihaoyi#910

Basically we avoid using the statically typed `zipMap` methods which cap out at 22, and instead use an untyped `zipMapLong` method that works with `IndexedSeq[T[Any]]` and `IndexedSeq[Any]`s. This could conceivably result in runtime errors if the macro has bugs, but assuming the macro is correct then it's as safe for the user as the existing macro is

By removing the old implementation, we are able to cut out a considerable amount of complexity from the codebase while still maintaining the existing semantics. Performance is probably better after, since we're just removing a layer of indirection: previously we ended up going through this dynamic `Seq[Any]` anyway, we just had a statically-typed facade wrapping it

Tested with a unit test, also tested manually in the `scratch` folder on the following build:

```scala
def t1 = T{ 1 }
def t2 = T{ 2 }
def t3 = T{ 3 }
def t4 = T{ 4 }
def t5 = T{ 5 }
def t6 = T{ 6 }
def t7 = T{ 7 }
def t8 = T{ 8 }
def t9 = T{ 9 }
def t10 = T{ 10 }
def t11 = T{ 11 }
def t12 = T{ 12 }
def t13 = T{ 13 }
def t14 = T{ 14 }
def t15 = T{ 15 }
def t16 = T{ 16 }
def t17 = T{ 17 }
def t18 = T{ 18 }
def t19 = T{ 19 }
def t20 = T{ 20 }
def t21 = T{ 21 }
def t22 = T{ 22 }
def t23 = T{ 23 }

def sum = T{
  t1() + t2() + t3() + t4() + t5() + t6() + t7() + t8() + t9() + t10() + t11() + t12() +
  t13() + t14() + t15() + t16() + t17() + t18() + t19() + t20() + t21() + t22() + t23()
}
```

This passes on this PR, fails on master.
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.

Targets cannot have more than 22 inputs
2 participants