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

Feature/multiple return values invariants #312

Conversation

phischu
Copy link
Collaborator

@phischu phischu commented Nov 16, 2023

No description provided.

@@ -108,7 +108,7 @@ class Repl(driver: Driver) extends REPL[Tree, EffektConfig, EffektError] {
runFrontend(StringSource(""), module.make(UnitLit()), config) { cu =>
module.definitions.foreach {
case u: Def =>
outputCode(DeclPrinter(context.symbolsOf(u)), config)
outputCode(DeclPrinter(List(u.id.symbol)), config)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is confusing to me, doesn't u.id.symbol already sometimes return a list?

case Some(b: BlockSymbol) =>
Some(context.blockTypeOf(b))
Some(b, context.blockTypeOf(b))
case t =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

As said before, did you check this works with the REPL? I would guess it does not print anything. Also it should probably be foreach and not map.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Prints nothing in the repl because d.id is empty. In case of ValDef we need d.binders.map(_.id)

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could in principle match on d and special case if it is a ValDef.

(ids zip tpes) foreach { (id, tpe) =>
Context.define(id, ValBinder(Context.nameFor(id), tpe, d))
binders.foreach { case source.ValueParam(id, tpe) =>
Context.define(id, ValBinder(Name.local(id), tpe.map(resolve), d))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change from nameFor to local intentional?

idDef ~ (`:` ~/> valueType).? ^^ { case id ~ tpe => (id, tpe)}
lazy val valDef: P[Def] =
`val` ~> someSep(valueParamOpt, `,`) ~ (`=` ~/> stmt) ^^ {
case binders ~ binding => ValDef(binders, binding)
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks, much better


def symbolsOf(tree: source.Definition): List[Symbol] =
tree.ids.map(symbolOf)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you check where this was used in master?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This function was written by me to get the list of symbols. It was only used in a few places.

Copy link
Collaborator

@b-studios b-studios left a comment

Choose a reason for hiding this comment

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

Looks good to me for now, but can you briefly answer the questions (if you haven't done so already)?

@b-studios
Copy link
Collaborator

/cc @denhie this is important for you

@denhie denhie merged commit d2e089b into feature/multiple-return-values Nov 16, 2023
0 of 2 checks passed
@b-studios
Copy link
Collaborator

Thanks for merging, but please let us answer the questions, still

@phischu phischu deleted the feature/multiple-return-values-invariants branch November 16, 2023 16:18
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.

3 participants