Skip to content

Cleanup handling of Contexts in RunInfo and Implicits #3679

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

Closed
wants to merge 1 commit into from

Conversation

smarter
Copy link
Member

@smarter smarter commented Dec 19, 2017

In several situations, instead of the common pattern in the compiler of
using the enclosing Context available coming from an implicit method
parameter, we used some other Context. Changing this is necessary for #2922
to go through. #2922 is currently on hold and may never get in but the
Context handling simplifications seem worth having anyway:

  • RunInfo does not actually need to keep a reference to the root Context:
    It was previously used by ImplicitRunInfo#implicitScope, but can be
    replaced by using the Context from the enclosing scope (which was already
    used in this method with the name liftingCtx).
  • OfTypeImplicits does not need to keep a reference to the root Context
    either, we can just replace its lazy vals by defs that cache their results.
  • ContextualImplicits does need to keep track of the Context from which
    its implicit references come from, but this Context itself does not have
    to be available implicitly in the class, instead the Context from the
    enclosing scope is used.

This also requires replacing OfTypeImplicits#toString and
ContextualImplicits#toString by a toText method in Printer to get access
to a Context.

In several situations, instead of the common pattern in the compiler of
using the enclosing Context available coming from an implicit method
parameter, we used some other Context. Changing this is necessary for scala#2922
to go through. scala#2922 is currently on hold and may never get in but the
Context handling simplifications seem worth having anyway:

- RunInfo does not actually need to keep a reference to the root Context:
  It was previously used by `ImplicitRunInfo#implicitScope`, but can be
  replaced by using the Context from the enclosing scope (which was already
  used in this method with the name `liftingCtx`).
- OfTypeImplicits does not need to keep a reference to the root Context
  either, we can just replace its lazy vals by defs that cache their results.
- ContextualImplicits does need to keep track of the Context from which
  its implicit references come from, but this Context itself does not have
  to be available implicitly in the class, instead the Context from the
  enclosing scope is used.

This also requires replacing OfTypeImplicits#toString and
ContextualImplicits#toString by a toText method in Printer to get access
to a Context.
@smarter smarter requested a review from odersky December 19, 2017 18:02
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 am uneasy about these changes because it significantly increases the possible error scenarios. In each of these cases we compute these conceptually global data structures from the context where they are first needed (which is basically arbitrary). So we'd have to prove that the result of these computations does not depend in any way on the context, and we'd have to somehow maintain that invariant in the future.

By contrast, what is the problem of keeping a link to the global context? I would imagine that the context is reachable via outer pointers anyway as long as the RunInfo itself is reachable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants