-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Check for tasty error in template trees. #22867
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
Conversation
15a44fb
to
fe86aee
Compare
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.
Otherwise LGTM.
@@ -654,6 +668,7 @@ object Semantic: | |||
val methodType = atPhaseBeforeTransforms { meth.info.stripPoly } | |||
var allArgsHot = true | |||
val allParamTypes = methodType.paramInfoss.flatten.map(_.repeatedToSingle) | |||
assert(allParamTypes.size == args.size) |
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.
Please make this an Internal Error instead of an assertion.
@@ -43,6 +46,7 @@ object Util: | |||
case Apply(fn, args) => | |||
val argTps = fn.tpe.widen match | |||
case mt: MethodType => mt.paramInfos | |||
assert(args.size == argTps.size) |
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.
Same as above, Internal Error.
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.
LGTM, thank you @lidaisy !
fe86aee
to
ccd9c22
Compare
@liufengyun @olhotak Can we merge this? |
Still waiting for a minor tweak. Please don't merge yet. |
ccd9c22
to
884436f
Compare
@tgodzik This is now ready to be merged, but CI failed seemingly due to a flaky test. Need to re-run CI. |
No description provided.