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.

Sign up for GitHub

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

Jump to bottom

Error on unused lets #1163

Merged
merged 5 commits into from Mar 15, 2024
Merged

Error on unused lets #1163

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a4564f0
Error on unused lets
johnynek Mar 5, 2024
65317d8
some minor changes from self code review
johnynek Mar 11, 2024
fe2b1ef
don't root synthetics
johnynek Mar 11, 2024
ca7746c
ban non-binding patterns, add tests
johnynek Mar 15, 2024
fc46ced
remove unused code
johnynek Mar 15, 2024
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
2 changes: 2 additions & 0 deletions core/src/main/resources/bosatsu/predef.bosatsu
View file
Open in desktop
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
7 changes: 7 additions & 0 deletions core/src/main/scala/org/bykn/bosatsu/Evaluation.scala
View file
Open in desktop
Expand Up @@ -104,6 +104,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
2 changes: 0 additions & 2 deletions core/src/main/scala/org/bykn/bosatsu/Indented.scala
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
131 changes: 101 additions & 30 deletions core/src/main/scala/org/bykn/bosatsu/PackageCustoms.scala
View file
Open in desktop
Expand Up @@ -17,15 +17,18 @@ 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 +62,39 @@ 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](te: TypedExpr[A]): VState[TypedExpr[A]] =
Copy link
Owner Author

Choose a reason for hiding this comment

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

This should be moved to TypedExpr.

te.traverseUp[VState] {
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)
}

private def usedGlobals[A](pack: Package.Typed[A]): Set[(PackageName, Identifier)] = {
val usedValuesSt: VState[Unit] =
pack.program.lets.traverse_ { case (_, _, te) => 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 +130,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 +233,66 @@ 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
pack.program.lets.collect { case (b, _, _) if b.asString.startsWith("_") => Right(b) } :::
Copy link
Owner Author

Choose a reason for hiding this comment

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

this should really be collecting all synthentic names and ignoring any synthetic name.

This probably should not be a root, just filtered from the errors. synthetic names are used to rewrite:

# top level
(a, b) = foo

into:

_a = foo
a = match _a:
  case (a, _): a
b = match _a:
  case (_, b): b

so, the only case where _a would be unused is for constructs like:

_ = foo(1)

where we intend this idiom to exist only to check that foo(1) type-checks, but can then be removed from runtime.

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] =
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, _, _) => !(bn.asString.startsWith("_") || 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))
}))
}
}
}
32 changes: 32 additions & 0 deletions core/src/main/scala/org/bykn/bosatsu/PackageError.scala
View file
Open in desktop
Expand Up @@ -886,4 +886,36 @@ 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
) = {
val prefix = sourceMap.headLine(inPack, None)
val (lm, _) = sourceMap.getMapSrc(inPack)
val di = (Doc.hardLine + Doc.intercalate(
Doc.line,
unusedLets.toList.map { case (b, _, _, region) =>
lm.showRegion(region, 2, errColor) match {
case Some(regionDoc) =>
Doc.text(b.sourceCodeRepr) + Doc.hardLine +
regionDoc
case None =>
Doc.text(b.sourceCodeRepr)
}
}
))
.nested(2)

val lets =
if (unusedLets.tail.lengthCompare(0) == 0) "let" else "lets"
(prefix + Doc.hardLine + Doc.text(
s"unused $lets of:"
) + di + Doc.hardLine).render(80)
}
}
}
2 changes: 2 additions & 0 deletions core/src/main/scala/org/bykn/bosatsu/PackageMap.scala
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion core/src/main/scala/org/bykn/bosatsu/SourceConverter.scala
View file
Open in desktop
Expand Up @@ -1218,7 +1218,7 @@ final class SourceConverter(

private def anonNameStrings(): Iterator[String] =
rankn.Type.allBinders.iterator
.map(_.name)
.map { bn => "_" + bn.name }
Copy link
Owner Author

Choose a reason for hiding this comment

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

this is a weird hidden hack: we are making names that are not parseable to make synthetic names.

Probably a better solution is to just make a new Bindable type called Synthetic, that has no Parser, so it is never the result of a parsing rather than backdoor it here.


private def unusedNames(allNames: Bindable => Boolean): Iterator[Bindable] =
anonNameStrings()
Expand Down
20 changes: 18 additions & 2 deletions core/src/main/scala/org/bykn/bosatsu/graph/Dag.scala
View file
Open in desktop
Expand Up @@ -78,7 +78,10 @@ object Dag {
m.iterator
.map { case (a, current) =>
val next = nfn(a).iterator.foldLeft(SortedSet.empty[A]) { (s, a) =>
s | m(a)
s | (m.get(a) match {
case Some(value) => value
case None => SortedSet.empty
})
}
a -> (current | next)
}
Expand Down Expand Up @@ -139,6 +142,19 @@ object Dag {
val depCache = MMap.empty[A, SortedSet[A]]
def deps(a: A): SortedSet[A] =
depCache.getOrElseUpdate(a, s.nfn(a).to(SortedSet))

}

def transitiveSet[A: Ordering](nodes: List[A])(nfn: A => Iterable[A]): SortedSet[A] = {
def loop(stack: List[A], inStack: SortedSet[A], reached: SortedSet[A]): SortedSet[A] =
stack match {
case head :: tail =>
val next = nfn(head).iterator.filterNot { n => inStack(n) || reached(n) }
.toList
.sorted
loop(next ::: tail, inStack ++ next, reached + head)
case Nil => reached
}

loop(nodes, SortedSet.empty, SortedSet.empty)
}
}