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

fix: Check if a PolyFunction TypeTree has no ByName parameters #21671

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

KacperFKorban
Copy link
Member

closes #21652

@KacperFKorban KacperFKorban marked this pull request as ready for review October 1, 2024 09:56
@KacperFKorban KacperFKorban requested a review from mbovel October 2, 2024 15:25
@mbovel
Copy link
Member

mbovel commented Oct 2, 2024

I see that there is already Checking.checkPolyFunctionType that is supposed to check that. So I'm wondering: couldn't we just comment the assertion in PolyFunctionOf.apply, and let PostTyper check that? I tried and it seems to work for this specific example:

-- Error: test.scala:1:8 -------------------------------------------------------
1 |def k: [A] => (=> A) => A = // error
  |        ^^^^^^^^^^^^^^^^^
  |Implementation restriction: PolyFunction apply must have exactly one parameter list and optionally type arguments. No by-name nor varags are allowed.
1 error found

@KacperFKorban
Copy link
Member Author

Good idea, less code is always better. I'm not sure about removing the assertion entirely. These types of assertions are pretty useful so that when something crashes, the error is thrown pretty early and not in an unrelated phase. Maybe just the byname and varargs part?

@KacperFKorban
Copy link
Member Author

Hmmm, actually it doesn't work for me. It compiles without any errors by default and crashed after some later phase with -Ycheck:all

@mbovel
Copy link
Member

mbovel commented Oct 3, 2024

Strange, have we tried the same change?

➜  ~/dotty git:(main) ✗ cat 21671.scala    
def k: [A] => (=> A) => A = // error
   [A] => a => a
➜  ~/dotty git:(main) ✗ git rev-parse HEAD    
c33276637e39e71c13e049d907b239d03a98ad5d
➜  ~/dotty git:(main) ✗ git --no-pager diff           
diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala
index f95bb3cea3..c5b7aa5897 100644
--- a/compiler/src/dotty/tools/dotc/core/Definitions.scala
+++ b/compiler/src/dotty/tools/dotc/core/Definitions.scala
@@ -1221,7 +1221,7 @@ class Definitions {
 
     /** Creates a refined `PolyFunction` with an `apply` method with the given info. */
     def apply(mt: MethodOrPoly)(using Context): Type =
-      assert(isValidPolyFunctionInfo(mt), s"Not a valid PolyFunction refinement: $mt")
+      //assert(isValidPolyFunctionInfo(mt), s"Not a valid PolyFunction refinement: $mt")
       RefinedType(PolyFunctionClass.typeRef, nme.apply, mt)
 
     /** Matches a refined `PolyFunction` type and extracts the apply info.
➜  ~/dotty git:(main) ✗ sbt "scalac 21671.scala"
...
-- Error: 21671.scala:1:8 ------------------------------------------------------
1 |def k: [A] => (=> A) => A = // error
  |        ^^^^^^^^^^^^^^^^^
  |Implementation restriction: PolyFunction apply must have exactly one parameter list and optionally type arguments. No by-name nor varags are allowed.
...

@KacperFKorban
Copy link
Member Author

Oooh, actually nevermind. It works, but the isValidPolyFunctionInfo is reused in Checking, so just removing the by-name and vararg checks doesn't work.

@mbovel mbovel enabled auto-merge October 3, 2024 10:55
@mbovel mbovel merged commit 5bbbdce into scala:main Oct 3, 2024
25 checks passed
@mbovel mbovel deleted the fix-i21652 branch October 3, 2024 11:35
@WojciechMazur WojciechMazur added this to the 3.6.0 milestone Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants