Fix #15764: Warn for problematic parameter overriding

[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.

[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 https://github.com/lampepfl/dotty/issues/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 https://github.com/lampepfl/dotty/pull/16066#issuecomment-1252908202, 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.