Code refactoring of initialization checker

[liufengyun]

Fix #15764: Warn for problematic parameter overriding

Check overriding of class parameters

This check issues a warning if the use of a class pamameter in the primary
constructor potentially has different semantics from its use in methods.
The subtle semantic difference is demonstrated by the following exmpale:

    class B(val y: Int):
      println(y)                   // 10
      foo()
      def foo() = println(y)       // 20

    class C(override val y: Int) extends B(10)

    new C(20)

A well-formed program should not depend on such subtle semantic differences.
Therefore, we detect and warn such subtle semantic differences in code.

This check depends on loading TASTY from libraries. It can be enabled with
the compiler option -Ysafe-init.

Edit: Given that #16096 already disallows the overriding of class parameters, we don't need this PR any more. It only contains refactoring now.

[olhotak]

I was surprised that the worklist was already not really a worklist, in that no new classes get added to it while it is processing existing classes. Given that this is the case, this simplification makes sense (i.e. getting rid of the mutable pendingTasks). But it would make sense to take the simplification a step further and get rid of the Worklist class altogether, keeping only its doTask method. Then here we would just do concreteClasses.filter(isConcreteClass).foreach(doTask). Also we should rename concreteClasses to just classes or classesToCheck or allClasses or ...

[odersky]

@liufengyun As far as I can see all changes are in the init checker only, which I am not very familiar with. Is there anything specific you want me to review?

[liufengyun]

@liufengyun As far as I can see all changes are in the init checker only, which I am not very familiar with. Is there anything specific you want me to review?

I just want to make sure if the check makes sense from a specification point of view. The check enforces that:

• A class parameter may only be overridden by a class parameter.
• If a class parameter overrides another class parameter, they should have the same value.

The two rules help avoid subtle and surprising semantics as shown in the example.

BTW, the new parameter overriding check is independent of the init checker.

[olhotak]

@liufengyun As far as I can see all changes are in the init checker only, which I am not very familiar with. Is there anything specific you want me to review?

@odersky I think the main question we want you to answer is whether we really do want a warning for this case, and whether being able to generate a warning for this case is worthwhile enough to justify the complexity of adding a static analysis of 350 LOC. It may be better to answer these questions in #15764 than here.

Although the static analysis is fairly simple in theory, there are many cases for the implementation to consider. Those cases do mirror those of the init checker, so they don't seem difficult for someone who is an expert in the init checker like @liufengyun .

@liufengyun If we do merge this, I think an explanation of the static analysis in theory (and the abstract domain) would be helpful, either in a big comment in the code or somewhere in a separate markdown file.

[odersky]

Hi Ondrej, Fengyun, OK, I responded on the PR. Let's discuss there. - Martin

…

On Tue, Sep 20, 2022 at 11:02 PM Ondřej Lhoták ***@***.***> wrote: @liufengyun <https://github.com/liufengyun> As far as I can see all changes are in the init checker only, which I am not very familiar with. Is there anything specific you want me to review? @odersky <https://github.com/odersky> I think the main question we want you to answer is whether we really do want a warning for this case, and whether being able to generate a warning for this case is worthwhile enough to justify the complexity of adding a static analysis of 350 LOC. It may be better to answer these questions in #15764 <#15764> than here. Although the static analysis is fairly simple in theory, there are many cases for the implementation to consider. Those cases do mirror those of the init checker, so they don't seem difficult for someone who is an expert in the init checker like @liufengyun <https://github.com/liufengyun> . @liufengyun <https://github.com/liufengyun> If we do merge this, I think an explanation of the static analysis in theory (and the abstract domain) would be helpful, either in a big comment in the code or somewhere in a separate markdown file. — Reply to this email directly, view it on GitHub <#16066 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGCKVTBALPMFZVEC7NUZ73V7IQ6JANCNFSM6AAAAAAQPQJ2YU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Martin Odersky Professor, Programming Methods Group (LAMP) Faculty IC, EPFL Station 14, Lausanne, Switzerland

[odersky]

Related: #16092

[liufengyun]

Given that #16096 already disallows the overriding of class parameters, we don't need this PR any more. It's more permissive than #16096 for legacy code, e.g., it handles seconary constructors and trait parameters. However, #16096 is much simpler and good enough for legacy code.

We can keep the refactoring improvements while dropping ParamOverridingCheck.scala and related tests.

[olhotak]

This is a nice cleanup of part of the init checker and the new comments are very helpful.

Before merging, I just suggest updating the PR title: it doesn't fix #15764 anymore.