Enable -Ycheck-init on community projects and CI #11385
Conversation
liufengyun
force-pushed
the
safe-object-init3
branch
from
2a9d600 to
f55587b
Compare
|
@nicolasstucki Could you have a look at the last commit a98d42a? |
liufengyun
force-pushed
the
safe-object-init3
branch
3 times, most recently
from
ec47a70 to
3ee75fb
Compare
| } | ||
| else { | ||
| state.visited = state.visited + eff | ||
| val state2: State = state.copy(path = state.path :+ eff.source) |
There was a problem hiding this comment.
In Scala when you copy are the mutable.Sets shared?
There was a problem hiding this comment.
Yes, the other fields take previous values, so they are shared.
| @@ -163,26 +155,29 @@ object Potentials { | |||
|
|
|||
| extension (pot: Potential) def toPots: Potentials = Potentials.empty + pot | |||
|
|
|||
| extension (ps: Potentials) def select (symbol: Symbol, source: Tree)(using Context): Summary = | |||
| ps.foldLeft(Summary.empty) { case ((pots, effs), pot) => | |||
| extension (ps: Potentials) def select (symbol: Symbol, source: Tree, selectEffect: Boolean = true)(using Context): Summary = | |||
There was a problem hiding this comment.
I don't really get selectEffect's purpose... :(
There was a problem hiding this comment.
Indeed we need a better name for that and add some documentation. It's used in potential expansion (selectEffect = false), where we don't want to trigger a duplicate method call or field access effect.
| assert(member.owner == currentClass, "owner = " + member.owner.show + ", current = " + currentClass.show) | ||
| summaryCache(member) = summary | ||
| } | ||
| case class Summary(pots: Potentials, effs: Effects) { |
There was a problem hiding this comment.
What is the difference between object and case class?
There was a problem hiding this comment.
Here we use explicit class for Summary, instead of putting them in a tuple (Potentials, Effects). The former is expected to be more maintainable.
|
I think the rest are just converting from old code to new structure, so nothing much I could say... |
liufengyun
force-pushed
the
safe-object-init3
branch
from
079baf2 to
defaffb
Compare
|
test performance please |
|
performance test scheduled: 2 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/11385/ to see the changes. Benchmarks is based on merging with master (4d08ccd) |
liufengyun
assigned nicolasstucki and natsukagami and unassigned liufengyun and nicolasstucki
|
@natsukagami I've addressed your comments. If it looks good to you, please approve so that we can move forward (unfortunately I cannot approve my own PR). |
- Make summary a proper class - Rename isInternal to hasSource - Refactor: remove useless parameter - Split effect check into multiple methods - Refactor ignored methods handling Ignore constructor call on Any, Object and AnyVal - Fix missing return
Handle NoPrefix properly
Otherwise, looking up members will crash the compiler
The owners are different for the patches, and they are always safe.
Shapeless enabled `-Xfatal-warings`, the warnings make the CI fail.
In the following test: - tests/pos/i3130b.scala If we add `transparent`, then everything is OK. The reason is that we set `Symbol.defTree` systematically in PostTyper. Now the inlining happens after PostTyper, thus `defTree` is not properly set for inlined definitions. To compensate, we ensure that `defTree` is set in the ReTyper. The IninerTyper extends ReTyper, thus it fixes the problem. Doing the fix directly in InlinerTyper, however, does not pass the CI. The reason is that `-Ycheck:all` will run `TreeChecker` which will make tree bindings get lost.
cats enables -Xfatal-warnings and we don't want to change the library to create conflicts with upstream.
To avoid unnecessary conflicts, we disable -Ycheck-init on shapeless.
liufengyun
force-pushed
the
safe-object-init3
branch
from
defaffb to
db19f60
Compare
|
rebased due to conflicts in |
|
Merge now to avoid conflicts and make it easier for the work on initialization. |
Cherry-picked from #10627