Skip to content
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

Fix checking ctx to carry correct modes #15350

Merged
merged 5 commits into from
Jun 21, 2022
Merged

Conversation

noti0na1
Copy link
Member

@noti0na1 noti0na1 commented Jun 1, 2022

In TreeChecker, the context for checking is not initialized properly. val checkingCtx uses setMode(Mode.ImplicitsEnabled), which removes other modes in the ctx.

This causes Mode.SafeNulls removed from the context. So the checking for explicit nulls tests is always running with unsafe nulls enabled!!

This PR fixs the checking ctx and some other bugs exposed by the correct checking:

  • Remove Imports during Erasure, so all other phases before it can be checked correctly
  • Cast the last expression of a block to the expected type,
    if unsafe nulls is enabled inside but not enabled outside
    and the type does not conform the expected type without unsafe nulls.
    See: tests/explicit-nulls/pos/unsafe-block.scala
  • Use unsafe nulls when forwarding a member before erasure if the target is Java defined
  • Cast null to AnyRef when generating testNotNull
  • When creating nameRef in SyntheticMembers.scala, cast the result of name to String if isJavaEnumValue

#15096 is blocked by this PR, #14946 will also wait for this

@noti0na1
Copy link
Member Author

noti0na1 commented Jun 2, 2022

test performance please

@dottybot
Copy link
Member

dottybot commented Jun 2, 2022

performance test scheduled: 1 job(s) in queue, 0 running.

@noti0na1 noti0na1 self-assigned this Jun 2, 2022
@dottybot
Copy link
Member

dottybot commented Jun 2, 2022

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/15350/ to see the changes.

Benchmarks is based on merging with main (42b5941)

@olhotak olhotak requested a review from odersky June 3, 2022 08:51
@noti0na1 noti0na1 assigned odersky and unassigned noti0na1 Jun 9, 2022
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

I think it's better to drop imports in Erasure during the regular tree pass. E.g. add:

  override def typedImport(imp: untpd.Import, sym: Symbol)(using Context) = EmptyTree

@odersky odersky assigned noti0na1 and unassigned odersky Jun 10, 2022
@noti0na1 noti0na1 force-pushed the fix-check-ctx branch 2 times, most recently from 4833ebc to 0cd0778 Compare June 15, 2022 23:12
@noti0na1 noti0na1 requested a review from odersky June 16, 2022 07:45
@noti0na1
Copy link
Member Author

test performance please

@dottybot
Copy link
Member

performance test scheduled: 3 job(s) in queue, 1 running.

@noti0na1 noti0na1 assigned odersky and unassigned noti0na1 Jun 16, 2022
@noti0na1
Copy link
Member Author

Hi @odersky , I removed the DropImports phase and drop imports in Erasure now. I also fixed two other bugs related to checking since last review.

This PR is ready for another review now. Thanks

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/15350/ to see the changes.

Benchmarks is based on merging with main (140693d)

Copy link
Contributor

@olhotak olhotak left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

val expr1 = typedExpr(tree.expr, pt.dropIfProto)(using exprCtx)
var expr1 = typedExpr(tree.expr, pt.dropIfProto)(using exprCtx)

// If unsafe nulls is enabled inside a block but not enabled outside
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment does not correspond to the code. exprCtx is the context inside the block valid for the final expression. ctx is the context outside the block. The cast is performed if unsafeNulls is disabled inside the block but enabled around it. Is this what's intended? Either the comment or the code has to be fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the comment is correct: !exprCtx.mode.is(Mode.SafeNulls) indicates SafeNulls is disabled inside the block, which means unsafeNulls is enabled here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The confusion is that the mode is SafeNulls which is a negation of unsafeNulls. I think the comment correctly corresponds to the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, my bad. I misread the code.

@odersky odersky assigned noti0na1 and unassigned odersky Jun 20, 2022
@odersky odersky merged commit 0059d1d into scala:main Jun 21, 2022
@odersky odersky deleted the fix-check-ctx branch June 21, 2022 07:44
@Kordyjan Kordyjan added this to the 3.2.1 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants