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 #19202: Passing NotNullInfos to a mutable field of a Completer #19463

Conversation

noti0na1
Copy link
Member

Fix #19202

Completing a Def requires the new NotNullInfos from previous statements.
Originally, we created a new Completer with the new creation ctx in adaptCreationContext.
However, we should not create a regular Completer for TypeDefs.

This PR fixes the issue by creating a mutable field for NotNullInfos in Completer,
and the field is updated in adaptCreationContext now.

There may be a better solution without the mutable field.

@dwijnand
Copy link
Member

Shall we try giving all the completers a newLikeThis method?

@smarter
Copy link
Member

smarter commented Jan 16, 2024

Yeah that might be the most sensible approach here, ideally we'd have a base class AbstractCompleter that keeps it abstract too

@smarter
Copy link
Member

smarter commented Jan 16, 2024

(or keep Completer with the same name but make it abstract and add a concrete Term completer?)

@noti0na1
Copy link
Member Author

noti0na1 commented Jan 17, 2024

LazyType, Completer and its subclasses (ClassCompleter and TypeDefCompleter) all have private mutable fields,
which makes it difficult to manage the copy and inti.

So, I think it is ok to keep NotNullInfos as a mutable field for now.

@noti0na1 noti0na1 force-pushed the create-correct-completer-when-passing-notnullinfos branch from b4a293e to bc057db Compare January 19, 2024 00:00
@@ -784,12 +785,18 @@ class Namer { typer: Typer =>

protected def localContext(owner: Symbol): FreshContext = ctx.fresh.setOwner(owner).setTree(original)

private var myNotNullInfos: List[NotNullInfo] | Null = null

/** The context with which this completer was created */
Copy link
Member

Choose a reason for hiding this comment

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

The comment should mention that this is affected by setNotNullInfos

@noti0na1 noti0na1 enabled auto-merge January 19, 2024 11:21
@noti0na1 noti0na1 merged commit 0b7a785 into scala:main Jan 19, 2024
19 checks passed
@noti0na1 noti0na1 deleted the create-correct-completer-when-passing-notnullinfos branch January 19, 2024 12:57
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
WojciechMazur added a commit that referenced this pull request Jun 28, 2024
…mpleter" to LTS (#20841)

Backports #19463 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
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.

scala.MatchError dotty.tools.dotc.typer.Namer$Completer.typeSig(Namer.scala:798)
4 participants