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

Error on unused lets #1163

Merged
merged 5 commits into from Mar 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions bench/src/main/scala/org/bykn/bosatsu/TestBench.scala
Expand Up @@ -65,7 +65,7 @@ gauss$n = range($n).foldLeft(0, add)
val c = compiled0
val ev = Evaluation(c._1, Predef.jvmExternals)
// run the evaluation
val _ = ev.evaluateLast(c._2).get._1.value
val _ = ev.evaluateMain(c._2).get._1.value
()
}

Expand All @@ -76,7 +76,7 @@ gauss$n = range($n).foldLeft(0, add)
val c = compiled1
val ev = Evaluation(c._1, Predef.jvmExternals)
// run the evaluation
val _ = ev.evaluateLast(c._2).get._1.value
val _ = ev.evaluateMain(c._2).get._1.value
()
}

Expand Down Expand Up @@ -153,7 +153,7 @@ max_pal = match max_pal_opt:
val c = compiled2
val ev = Evaluation(c._1, Predef.jvmExternals)
// run the evaluation
val _ = ev.evaluateLast(c._2).get._1.value
val _ = ev.evaluateMain(c._2).get._1.value
()
}
}
2 changes: 2 additions & 0 deletions core/src/main/resources/bosatsu/predef.bosatsu
Expand Up @@ -46,6 +46,7 @@ export (
Dict,
add,
add_key,
build_List,
char_to_String,
cmp_Int,
concat,
Expand All @@ -56,6 +57,7 @@ export (
eq_Int,
flat_map_List,
foldLeft,
foldr_List,
gcd_Int,
get_key,
int_loop,
Expand Down
14 changes: 7 additions & 7 deletions core/src/main/scala/org/bykn/bosatsu/Evaluation.scala
Expand Up @@ -74,13 +74,6 @@ case class Evaluation[T](pm: PackageMap.Typed[T], externals: Externals) {
}
)

def evaluateLast(p: PackageName): Option[(Eval[Value], Type)] =
for {
pack <- pm.toMap.get(p)
(name, _, tpe) <- pack.program.lets.lastOption
value <- evaluate(p).get(name)
} yield (value, tpe.getType)

// TODO: this only works for lets, not externals
def evaluateName(
p: PackageName,
Expand All @@ -104,6 +97,13 @@ case class Evaluation[T](pm: PackageMap.Typed[T], externals: Externals) {
value <- evaluate(p).get(name)
} yield value

def evaluateMain(p: PackageName): Option[(Eval[Value], Type)] =
for {
pack <- pm.toMap.get(p)
(name, _, te) <- Package.mainValue(pack)
value <- evaluate(p).get(name)
} yield (value, te.getType)

/* TODO: this is useful for debugging, but we should probably test it and write a parser for the
* list syntax

Expand Down
3 changes: 3 additions & 0 deletions core/src/main/scala/org/bykn/bosatsu/Identifier.scala
Expand Up @@ -128,4 +128,7 @@ object Identifier {

implicit def ordering[A <: Identifier]: Ordering[A] =
order[A].toOrdering

def synthetic(name: String): Bindable =
Name("_" + name)
}
2 changes: 0 additions & 2 deletions core/src/main/scala/org/bykn/bosatsu/Indented.scala
Expand Up @@ -4,8 +4,6 @@ import org.typelevel.paiges.{Doc, Document}

import cats.parse.{Parser => P}

import cats.implicits._

case class Indented[T](spaces: Int, value: T) {
require(spaces > 0, s"need non-empty indentation: $spaces")
}
Expand Down
10 changes: 4 additions & 6 deletions core/src/main/scala/org/bykn/bosatsu/MainModule.scala
Expand Up @@ -521,13 +521,13 @@ abstract class MainModule[IO[_]](implicit
ps: List[(Path, PackageName)]
): IO[(PackageName, Option[Bindable])] =
ps.collectFirst { case (path, pn) if path == mainFile => pn } match {
case Some(p) => moduleIOMonad.pure((p, None))
case None =>
moduleIOMonad.raiseError(
new Exception(
s"could not find file $mainFile in parsed sources"
)
)
case Some(p) => moduleIOMonad.pure((p, None))
}
}

Expand Down Expand Up @@ -1018,16 +1018,14 @@ abstract class MainModule[IO[_]](implicit
def runEval: IO[(Evaluation[Any], Output.EvaluationResult)] = withEC {
implicit ec =>
for {
pn <- inputs.packMap(this, List(mainPackage), errColor)
(packs, names) = pn
mainPackageNameValue <- mainPackage.getMain(names)
(mainPackageName, value) = mainPackageNameValue
(packs, names) <- inputs.packMap(this, List(mainPackage), errColor)
(mainPackageName, value) <- mainPackage.getMain(names)
out <-
if (packs.toMap.contains(mainPackageName)) {
val ev = Evaluation(packs, Predef.jvmExternals)

val res = value match {
case None => ev.evaluateLast(mainPackageName)
johnynek marked this conversation as resolved.
Show resolved Hide resolved
case None => ev.evaluateMain(mainPackageName)
case Some(ident) => ev.evaluateName(mainPackageName, ident)
}

Expand Down
5 changes: 5 additions & 0 deletions core/src/main/scala/org/bykn/bosatsu/Package.scala
Expand Up @@ -97,6 +97,11 @@ object Package {
te.getType == Type.TestType
}.lastOption

def mainValue[A](
tp: Typed[A]
): Option[(Identifier.Bindable, RecursionKind, TypedExpr[A])] =
tp.program.lets.lastOption

/** Discard any top level values that are not referenced, exported, the final
* test value, or the final expression
*
Expand Down
109 changes: 77 additions & 32 deletions core/src/main/scala/org/bykn/bosatsu/PackageCustoms.scala
@@ -1,6 +1,5 @@
package org.bykn.bosatsu

import cats.Monad
import cats.data.{
Chain,
NonEmptyList,
Expand All @@ -16,16 +15,18 @@ import scala.collection.immutable.SortedSet
import cats.syntax.all._
import org.bykn.bosatsu.Referant.Constructor
import org.bykn.bosatsu.Referant.DefinedT
import org.bykn.bosatsu.TypedExpr.Match
import org.bykn.bosatsu.Identifier.Bindable
import org.bykn.bosatsu.graph.Dag

/** This checks the imports and exports of compiled packages and makes sure they
* are valid
*/
object PackageCustoms {
def apply[A](
def apply[A: HasRegion](
pack: Package.Typed[A]
): ValidatedNec[PackageError, Package.Typed[A]] =
checkValuesHaveExportedTypes(pack.name, pack.exports) *>
noUselessBinds(pack) *>
allImportsAreUsed(pack)

private def removeUnused[A](
Expand Down Expand Up @@ -59,6 +60,16 @@ object PackageCustoms {
pack.copy(imports = i)
}

private type VSet = Set[(PackageName, Identifier)]
Copy link
Owner Author

Choose a reason for hiding this comment

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

Identifier should be changed to Bindable for unused names.

Copy link
Owner Author

Choose a reason for hiding this comment

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

We don't check that types are used. There should be a similar check for a type that is never used.

private type VState[X] = State[VSet, X]

private def usedGlobals[A](pack: Package.Typed[A]): Set[(PackageName, Identifier)] = {
val usedValuesSt: VState[Unit] =
pack.program.lets.traverse_ { case (_, _, te) => TypedExpr.usedGlobals(te) }

usedValuesSt.runS(Set.empty).value
}

private def allImportsAreUsed[A](
pack: Package.Typed[A]
): ValidatedNec[PackageError, Package.Typed[A]] = {
Expand Down Expand Up @@ -94,34 +105,7 @@ object PackageCustoms {

if (impValues.isEmpty && impTypes.isEmpty) Validated.valid(pack)
else {
type VSet = Set[(PackageName, Identifier)]
type VState[X] = State[VSet, X]
val usedValuesSt: VState[Unit] =
pack.program.lets.traverse_ { case (_, _, te) =>
te.traverseUp {
case g @ TypedExpr.Global(p, n, _, _) =>
State(s => (s + ((p, n)), g))
case m @ Match(_, branches, _) =>
branches
.traverse_ { case (pat, _) =>
pat
.traverseStruct[
VState,
(PackageName, Identifier.Constructor)
] { (n, parts) =>
State.modify[VSet](_ + n) *>
parts.map { inner =>
Pattern.PositionalStruct(n, inner)
}
}
.void
}
.as(m)
case te => Monad[VState].pure(te)
}
}

val usedValues = usedValuesSt.runS(Set.empty).value
val usedValues = usedGlobals(pack)

val usedTypes: Set[Type.Const] =
pack.program.lets.iterator
Expand Down Expand Up @@ -224,4 +208,65 @@ object PackageCustoms {
case Some(nel) => Validated.invalid(nel)
}
}
}

private def noUselessBinds[A: HasRegion](pack: Package.Typed[A]): ValidatedNec[PackageError, Unit] = {
type Node = Either[pack.exports.type, Bindable]
implicit val ordNode: Ordering[Node] =
new Ordering[Node] {
def compare(x: Node, y: Node): Int =
x match {
case Right(bx) =>
y match {
case Left(_) => 1
case Right(by) =>
Ordering[Identifier].compare(bx, by)
}
case Left(_) =>
y match {
case Left(_) => 0
case Right(_) => -1
}
}
}

val exports: Node = Left(pack.exports)
val roots: List[Node] =
(exports ::
johnynek marked this conversation as resolved.
Show resolved Hide resolved
Package.testValue(pack).map { case (b, _, _) => Right(b) }.toList :::
Package.mainValue(pack).map { case (b, _, _) => Right(b) }.toList).distinct

val bindMap: Map[Bindable, TypedExpr[A]] =
pack.program.lets.iterator.map { case (b, _, te) => (b, te) }.toMap

def internalDeps(te: TypedExpr[A]): Set[Bindable] =
TypedExpr.usedGlobals(te).runS(Set.empty).value.collect {
case (pn, i: Identifier.Bindable) if pn == pack.name => i
}

def depsOf(n: Node): Iterable[Node] =
n match {
case Left(_) => pack.exports.flatMap {
case ExportedName.Binding(n, _) => Right(n) :: Nil
case _ => Nil
}
case Right(value) =>
bindMap.get(value) match {
case None => Nil
case Some(te) => internalDeps(te).map(Right(_))
}
}
val canReach: SortedSet[Node] = Dag.transitiveSet(roots)(depsOf _)

val unused = pack.program.lets.filter {
case (bn, _, _) => !canReach.contains(Right(bn))
}

NonEmptyList.fromList(unused) match {
case None => Validated.unit
case Some(value) =>
Copy link
Owner Author

Choose a reason for hiding this comment

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

We should add a test to exercise the error message from this.

Validated.invalidNec(PackageError.UnusedLets(pack.name, value.map { case (b, r, te) =>
(b, r, te, HasRegion.region(te))
}))
}
}
}
14 changes: 14 additions & 0 deletions core/src/main/scala/org/bykn/bosatsu/PackageError.scala
Expand Up @@ -886,4 +886,18 @@ object PackageError {
) + di + Doc.hardLine).render(80)
}
}

case class UnusedLets(
inPack: PackageName,
unusedLets: NonEmptyList[(Identifier.Bindable, RecursionKind, TypedExpr[Any], Region)]
) extends PackageError {
def message(
sourceMap: Map[PackageName, (LocationMap, String)],
errColor: Colorize
) =
UnusedLetError(
inPack,
unusedLets.map { case (b, _, _, r) => (b, r)}
).message(sourceMap, errColor)
}
}
2 changes: 2 additions & 0 deletions core/src/main/scala/org/bykn/bosatsu/PackageMap.scala
Expand Up @@ -426,6 +426,8 @@ object PackageMap {
// We have a result, which we can continue to check
val pack = Package(nm, imps, exports, program)
val res = (fte, pack)
// We have to check the "customs" before any normalization
// or optimization
PackageCustoms(pack) match {
case Validated.Valid(p1) => Ior.right((fte, p1))
case Validated.Invalid(errs) =>
Expand Down
24 changes: 16 additions & 8 deletions core/src/main/scala/org/bykn/bosatsu/SourceConverter.scala
Expand Up @@ -1216,13 +1216,9 @@ final class SourceConverter(
private lazy val localTypeEnv: Result[TypeEnv[Any]] =
toTypeEnv.map(p => importedTypeEnv ++ TypeEnv.fromParsed(p))

private def anonNameStrings(): Iterator[String] =
rankn.Type.allBinders.iterator
.map(_.name)

private def unusedNames(allNames: Bindable => Boolean): Iterator[Bindable] =
anonNameStrings()
.map(Identifier.Name(_))
rankn.Type.allBinders.iterator
.map { b => Identifier.synthetic(b.name) }
.filterNot(allNames)

/** Externals are not permitted to be shadowed at the top level
Expand Down Expand Up @@ -1405,6 +1401,12 @@ final class SourceConverter(
}
}

val noBinds: Result[Unit] = stmts.parTraverse_ {
case Bind(BindingStatement(b, d, _)) if b.names.isEmpty =>
SourceConverter.partial(SourceConverter.NonBindingPattern(b, d), ())
case _ => SourceConverter.successUnit
}

val flatIn: List[(Bindable, RecursionKind, Flattened)] =
SourceConverter.makeLetsUnique(flatList) { (bind, _) =>
// rename all but the last item
Expand Down Expand Up @@ -1461,6 +1463,7 @@ final class SourceConverter(
case (b, _, Right((_, d))) => Right(Right((b, d)))
}

noBinds.parProductR(
parFold(Set.empty[Bindable], withEx) { case (topBound, stmt) =>
stmt match {
case Right(Right((nm, decl))) =>
Expand Down Expand Up @@ -1513,8 +1516,7 @@ final class SourceConverter(
case Left(ExternalDef(n, _, _)) =>
(topBound + n, success(Nil))
}
}(SourceConverter.parallelIor)
.map(_.flatten)
}(SourceConverter.parallelIor)).map(_.flatten)
}

def toProgram(
Expand Down Expand Up @@ -1936,4 +1938,10 @@ object SourceConverter {
.render(80)
}
}

final case class NonBindingPattern(pattern: Pattern.Parsed, bound: Declaration) extends Error {
def message =
(Document[Pattern.Parsed].document(pattern) + Doc.text(" does not bind any names.")).render(80)
def region = bound.region
}
}