Skip to content

Fixed Missing warning for invalid recursive val #14598

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

Closed
wants to merge 1 commit into from

Conversation

gagandeepkalra
Copy link
Contributor

resolves #14429

cc. @anatoliykmetyuk

@gagandeepkalra gagandeepkalra changed the title fixed Missing warning for invalid recursive val Fixed Missing warning for invalid recursive val Mar 1, 2022
@griggt
Copy link
Contributor

griggt commented Mar 1, 2022

Also #12943 which already seems to be fixed.

@@ -83,7 +83,7 @@ class CheckLoopingImplicits extends MiniPhase:
checkNotLooping(t.rhs)
case _ =>

if sym.isOneOf(GivenOrImplicit | Lazy | ExtensionMethod) then
if sym.isOneOf(GivenOrImplicit | Lazy | ExtensionMethod) || mdef.isInstanceOf[ValDef] then
Copy link
Member

Choose a reason for hiding this comment

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

Is | Lazy still needed? Aren't all lazy values ValDefs?

@mbovel
Copy link
Member

mbovel commented Mar 14, 2022

About the error message:

  • Infinite loop in function body does not seem appropriate for vals.
  • Why don't we refer to the precise place where the recursion happens instead of embedding the function/val body in the error message?

Currently:

-- Warning: tests/neg-custom-args/fatal-warnings/i13011.scala:15:29 ------------
15 |  lazy val simple6: String = {  // error
   |                             ^
   |                             Infinite loop in function body
   |                             {
   |                               this.simple6
   |                               "aa"
   |                             }
16 |    this.simple6
17 |    "aa"
18 |  }

Instead, I would suggest:

-- Warning: tests/neg-custom-args/fatal-warnings/i13011.scala:16:9 -------------
16 |    this.simple6
   |    ^^^^^^^^^^^^
   |    Infinite loop

That could be achieved by simplifying checkNotSelfRef to:

def checkNotSelfRef(t: RefTree) =
  if t.symbol eq sym then
    report.warning("Infinite loop", t.srcPos)

Seems to work for for extension methods and givens as well:

-- Warning: tests/neg-custom-args/fatal-warnings/i13011.scala:22:41 ------------
22 |  extension (n: Int) def foo(): Unit = n.foo()
   |                                         ^^^
   |                                         Infinite loop
-- Warning: tests/neg-custom-args/fatal-warnings/i13011.scala:24:17 ------------
24 |  given x: Int = x
   |                 ^
   |                 Infinite loop

@mbovel
Copy link
Member

mbovel commented Mar 14, 2022

Minor: should we rename this phase to CheckInfiniteLoops as it's not just about implicits?

@mbovel
Copy link
Member

mbovel commented Sep 6, 2022

@gagandeepkalra do you plan to further work on this PR? It would be great to have it merged! 😃

@gagandeepkalra
Copy link
Contributor Author

Hi @mbovel, I'll get back to working this.

@ckipp01
Copy link
Member

ckipp01 commented May 10, 2023

Hi @mbovel, I'll get back to working this.

Hey @gagandeepkalra do you still plan on returning to this?

@mbovel mbovel self-assigned this Apr 26, 2024
@som-snytt
Copy link
Contributor

Closing in favor of Wsafe-init and/or -Ysafe-init-global, see the ticket.

@som-snytt som-snytt closed this Feb 6, 2025
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.

Missing warning for invalid recursive val.
5 participants