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(compiler): Throw an error when comparing an alias and a named type with the same name [LNG-231] #946

Merged
merged 6 commits into from
Oct 27, 2023

Conversation

DieMyst
Copy link
Member

@DieMyst DieMyst commented Oct 27, 2023

Description

Throw compilation error on a code like this:

alias NamedT: u32

func test():
  MyAbility = NamedT(randomField = "sf")

Implementation Details

Don't ignore named type comparison with aliases, improve type checking error.

Checklist

  • Corresponding issue has been created and linked in PR title.
  • Proposed changes are covered by tests.
  • Documentation has been updated to reflect the changes (if applicable).
  • I have self-reviewed my code and ensured its quality.
  • I have added/updated necessary comments to aid understanding.

Reviewer Checklist

  • Tests have been reviewed and are sufficient to validate the changes.
  • Documentation has been reviewed and is up to date.
  • Any questions or concerns have been addressed.

@linear
Copy link

linear bot commented Oct 27, 2023

LNG-231 Fix NamedType creation with improper types

aqua:

alias Troll: string

func test() -> i8:
  MyAbility = Troll(lololo = 1, trololo = 2)

  <- 42

compiles and produces air, but should not

also it seems to crash aqua vscode extension

@DieMyst DieMyst marked this pull request as ready for review October 27, 2023 09:28
@DieMyst DieMyst added the e2e Run e2e workflow label Oct 27, 2023
): State[X, Boolean] =
if (expected.acceptsValueOf(givenType)) State.pure(true)
else {
val expectedStr = alias.map(a => s"$a, that is an alias of '$expected'").getOrElse(expected.toString)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val expectedStr = alias.map(a => s"$a, that is an alias of '$expected'").getOrElse(expected.toString)
val expectedStr = alias.map(a => s"$a (an alias of '$expected')").getOrElse(expected.toString)

@@ -333,10 +333,12 @@ class TypesInterpreter[S[_], X](using
override def ensureTypeMatches(
token: Token[S],
expected: Type,
givenType: Type
givenType: Type,
alias: Option[String] = None
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 like adding this argument here. TypesAlgebra is the one that should know about type aliases, it should not get this info from outside, should it?
Maybe lets add a separate method resolveNamedTypeConstruction, which will take named type token and field types, then resolve the type and report a error if the type is not data or ability?

@InversionSpaces
Copy link
Contributor

Remove println in tests from previous PR here, please

@@ -58,6 +58,12 @@ class TypesInterpreter[S[_], X](using
report.error(token, s"Unresolved type").as(None)
}

def resolveNamedType(token: TypeToken[S]): State[X, Option[Type]] =
resolveType(token).flatMap(_.flatTraverse {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I believe we should move towards OptionT because we have F[Option[A]] almost all the time and it produces a lot of such flatMap(_.flatTraverse( chains. But I am not insisting on rewriting this right now

@@ -58,6 +58,12 @@ class TypesInterpreter[S[_], X](using
report.error(token, s"Unresolved type").as(None)
}

def resolveNamedType(token: TypeToken[S]): State[X, Option[Type]] =
Copy link
Contributor

@InversionSpaces InversionSpaces Oct 27, 2023

Choose a reason for hiding this comment

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

I believe result type is better to be NamedType instead of Type, or even AbilityType | StructType, to avoid pattern matching on call site

@DieMyst DieMyst enabled auto-merge (squash) October 27, 2023 11:14
@DieMyst DieMyst merged commit 38f7728 into main Oct 27, 2023
9 checks passed
@DieMyst DieMyst deleted the LNG-231-compare-aliases-and-struct-types branch October 27, 2023 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants