Skip to content

Fix remaining initialization warnings in bootstrapping #15682

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jul 17, 2022
Merged

Fix remaining initialization warnings in bootstrapping #15682

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c427046
Add test
liufengyun Jul 14, 2022
d7754d9
Refine error message
liufengyun Jul 14, 2022
3d28731
Use hot instead of fully initialized
liufengyun Jul 14, 2022
b6af7ee
Update check files
liufengyun Jul 14, 2022
80d9975
Fix crash in member selection from type cast
liufengyun Jul 14, 2022
920d4a9
Code refactoring
liufengyun Jul 14, 2022
41a4f71
Faster promotion
liufengyun Jul 14, 2022
dd1e724
Reclassify test
liufengyun Jul 14, 2022
b7dba0d
Rename variable
liufengyun Jul 14, 2022
4a4affd
Add assertions to tryPromote
liufengyun Jul 15, 2022
0ae7346
Fix typo in doc
liufengyun Jul 15, 2022
df875ee
Improve error messages
liufengyun Jul 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 28 additions & 14 deletions compiler/src/dotty/tools/dotc/transform/init/Semantic.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1138,20 +1138,34 @@ object Semantic:

val errors = Reporter.stopEarly {
for klass <- warm.klass.baseClasses if klass.hasSource do
for member <- klass.info.decls do
if !member.isType && !member.isConstructor && member.hasSource && !member.is(Flags.Deferred) then
if member.is(Flags.Method, butNot = Flags.Accessor) then
withTrace(Trace.empty) {
val args = member.info.paramInfoss.flatten.map(_ => ArgInfo(Hot, Trace.empty))
val res = warm.call(member, args, receiver = warm.klass.typeRef, superType = NoType)
res.promote("Cannot prove that the return value of " + member.show + " is hot. Found = " + res.show + ". ")
}
else
withTrace(Trace.empty) {
val res = warm.select(member, receiver = warm.klass.typeRef)
res.promote("Cannot prove that the field " + member.show + " is hot. Found = " + res.show + ". ")
}
end for
val obj = warm.objekt
val outer = obj.outer(klass)
val ctor = klass.primaryConstructor
val hotSegmentCountered = !ctor.hasSource || outer.isHot && {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I agree that this is correct but it is very subtle.

If A extends B, is it possible that hotSegmentCountered is true for the A part of an object but false for the B part of an object? In other words, if class A(a) extends B(b), is it possible for a (generally all arguments of A) to be hot but for b to somehow be cold? I would think that local reasoning would rule it out, but if it is possible, that might point us to possible flaws in the correctness reasoning. So I would suggest adding an assertion to say that if hotSegmentCountered is true for some subclass, then it also has to be true for all of the later super- baseclasses in the chain, just to catch some potential bugs in the reasoning.

Copy link
Contributor Author

@liufengyun liufengyun Jul 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is a valid concern, and it makes subtle assumptions about the ordering of the traversal. We've changed the logic in later commits to address this concern.

Edit: It's already in this commit, but we changed the name in a later commit to avoid misunderstanding. We do check each individual class separately now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why the ordering of the traversal would be relevant and from your comment I think we have different intuitions about why this code is correct. It would be good to unify those intuitions and document the unified understanding in a comment.

My understanding is one of local reasoning. If I write x = new Foo(a) and a is hot, then x is hot (assuming outer is also hot). If I have class Bar(a,b) extends Foo(a), then I can think of new Bar(a,b) as first creating a Foo(a) but then adding some more (Bar) stuff onto it. The Foo(a) part enjoys local reasoning: everything in it, including superclasses of Foo, should be all hot due to being instantiated in a hot environment. So it's just the Bar added on that we need to worry about, because it can access cold b. Calls of Foo methods can access b if they virtually dispatch to methods implemented in Bar, but that's OK because we'll analyze those overriding methods in Bar when we analyze Bar.

So the assertion is to confirm my understanding that of Foo(a) is instantiated in a hot environment then the constructors of superclasses of Foo also are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point --- actually the initial implementation tries to profit from the insight you mentioned above. However, the existence of traits and linearization complicate the logic.

In the end, I find it simpler to perform the check for all classes and not profit inheritance to skip some checks. The check is cheap, so it's not a problem.

So the assertion is to confirm my understanding that of Foo(a) is instantiated in a hot environment then the constructors of superclasses of Foo also are.

Assertions are always good. After linearization, adding the assertion incurs some complexity. I'll see if there is a simpler way to add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we added the assertion.

val ctorDef = ctor.defTree.asInstanceOf[DefDef]
val params = ctorDef.termParamss.flatten.map(_.symbol)
// We have cached all parameters on the object
params.forall(param => obj.field(param).isHot)
}

// If the outer and parameters of a class are all hot, then accessing fields and methods of the current
// segment of the object should be OK. They may only create problems via virtual method calls on `this`, but
// those methods are checked as part of the check for the class where they are defined.
if !hotSegmentCountered then
for member <- klass.info.decls do
if !member.isType && !member.isConstructor && member.hasSource && !member.is(Flags.Deferred) then
if member.is(Flags.Method, butNot = Flags.Accessor) then
withTrace(Trace.empty) {
val args = member.info.paramInfoss.flatten.map(_ => ArgInfo(Hot, Trace.empty))
val res = warm.call(member, args, receiver = warm.klass.typeRef, superType = NoType)
res.promote("Cannot prove that the return value of " + member.show + " is hot. Found = " + res.show + ". ")
}
else
withTrace(Trace.empty) {
val res = warm.select(member, receiver = warm.klass.typeRef)
res.promote("Cannot prove that the field " + member.show + " is hot. Found = " + res.show + ". ")
}
end for
end for
}

Expand Down