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

Exporting non-existing symbols fails with compiler error #7960

Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -971,8 +971,9 @@
- [Always persist `TRACE` level logs to a file][7825]
- [Downloadable VSCode extension][7861]
- [New `project/status` route for reporting LS state][7801]
- [Add Enso-specific assertions][7883])
- [Add Enso-specific assertions][7883]
- [Modules can be `private`][7840]
- [Export of non-existing symbols results in error][7960]

[3227]: https://github.com/enso-org/enso/pull/3227
[3248]: https://github.com/enso-org/enso/pull/3248
Expand Down Expand Up @@ -1119,6 +1120,7 @@
[7861]: https://github.com/enso-org/enso/pull/7861
[7883]: https://github.com/enso-org/enso/pull/7883
[7840]: https://github.com/enso-org/enso/pull/7840
[7960]: https://github.com/enso-org/enso/pull/7960

# Enso 2.0.0-alpha.18 (2021-10-12)

Expand Down
1 change: 0 additions & 1 deletion distribution/lib/Standard/Base/0.0.0-dev/src/Main.enso
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ from project.Data.Numbers export Float, Integer, Number
from project.Data.Range.Extensions export all
from project.Data.Statistics export all hiding to_moment_statistic, wrap_java_call, calculate_correlation_statistics, calculate_spearman_rank, calculate_correlation_statistics_matrix, compute_fold, empty_value, is_valid
from project.Data.Text.Extensions export all
from project.Data.Time.Conversions export all
Copy link
Member

Choose a reason for hiding this comment

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

This file does not exist, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yep.

It's my fault, I forgot to remove this export after removing the file, sorry.

Thanks for adding a pass that catches such errors! ❤️

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. This is why it is now removed - the compilation now fails with this pass.

from project.Errors.Problem_Behavior.Problem_Behavior export all
from project.Function export all
from project.Meta.Enso_Project export enso_project
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,10 @@ object ImportExport {

case class SymbolDoesNotExist(
symbolName: String,
moduleName: String
moduleOrTypeName: String
) extends Reason {
override def message: String =
s"The symbol $symbolName (module or type) does not exist in module $moduleName."
s"The symbol $symbolName (module, type, or constructor) does not exist in $moduleOrTypeName."
}

case class NoSuchConstructor(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
package org.enso.compiler.pass.analyse;

import java.util.ArrayList;
import java.util.List;
import java.util.UUID;
import org.enso.compiler.context.InlineContext;
import org.enso.compiler.context.ModuleContext;
import org.enso.compiler.core.IR;
import org.enso.compiler.core.ir.Expression;
import org.enso.compiler.core.ir.Module;
import org.enso.compiler.core.ir.expression.errors.ImportExport;
import org.enso.compiler.core.ir.module.scope.Export;
import org.enso.compiler.data.BindingsMap;
import org.enso.compiler.pass.IRPass;
import org.enso.interpreter.util.ScalaConversions;
import scala.collection.immutable.Seq;
import scala.jdk.javaapi.CollectionConverters;

/**
* This pass ensures that all the symbols that are exported exist. If not, an IR error is generated.
*/
public class ExportSymbolAnalysis implements IRPass {
public static final ExportSymbolAnalysis INSTANCE = new ExportSymbolAnalysis();
private UUID uuid;

private ExportSymbolAnalysis() {}

@Override
public UUID key() {
return null;
}

@Override
public void org$enso$compiler$pass$IRPass$_setter_$key_$eq(UUID v) {
this.uuid = v;
}

@Override
public Seq<IRPass> precursorPasses() {
List<IRPass> passes = List.of(
BindingAnalysis$.MODULE$,
ImportSymbolAnalysis$.MODULE$
);
return CollectionConverters.asScala(passes).toList();
Akirathan marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public Seq<IRPass> invalidatedPasses() {
return ScalaConversions.nil();
}

@Override
public Module runModule(Module moduleIr, ModuleContext moduleContext) {
List<Export> exportErrors = new ArrayList<>();
var bindingsMap = (BindingsMap) moduleIr.passData().get(BindingAnalysis$.MODULE$).get();

moduleIr.exports().foreach(export -> switch (export) {
case Export.Module exportMod -> {
var exportNameParts = exportMod.name().parts();
var symbolName = exportMod.name().parts().last();
assert exportNameParts.size() > 1;
var moduleOrTypeName = exportNameParts.apply(exportNameParts.size() - 2);
var foundResolvedExp = findResolvedExportForIr(export, bindingsMap);
if (foundResolvedExp == null) {
exportErrors.add(
ImportExport.apply(
symbolName,
new ImportExport.SymbolDoesNotExist(symbolName.name(), moduleOrTypeName.name()),
ImportExport.apply$default$3(),
ImportExport.apply$default$4()
)
);
} else {
if (exportMod.onlyNames().isDefined()) {
assert exportMod.onlyNames().isDefined();
var exportedSymbols = exportMod.onlyNames().get();
exportedSymbols.foreach(exportedSymbol -> {
var foundSymbols = foundResolvedExp.target().findExportedSymbolsFor(exportedSymbol.name());
if (foundSymbols.isEmpty()) {
exportErrors.add(
ImportExport.apply(
exportedSymbol,
new ImportExport.SymbolDoesNotExist(exportedSymbol.name(), moduleOrTypeName.name()),
ImportExport.apply$default$3(),
ImportExport.apply$default$4()
)
Comment on lines +85 to +90
Copy link
Member

Choose a reason for hiding this comment

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

It makes me sad to see this done in not-Scala, but I know this is the general direction. But it still makes me sad 🙁

Copy link
Member Author

Choose a reason for hiding this comment

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

Apart from some weird perks, like calling methods with default arguments, or methods on objects, there is nothing stopping us from calling Scala stuff from Java stuff anymore. It does not look nice, but it is the most viable compromise for now.

);
}
return null;
});
}
}
yield null;
}
default -> export;
});
return moduleIr.copy(
moduleIr.imports(),
exportErrors.isEmpty() ? moduleIr.exports() : CollectionConverters.asScala(exportErrors).toList(),
Akirathan marked this conversation as resolved.
Show resolved Hide resolved
moduleIr.bindings(),
moduleIr.location(),
moduleIr.passData(),
moduleIr.diagnostics(),
moduleIr.id()
);

}

@Override
public Expression runExpression(Expression ir, InlineContext inlineContext) {
return ir;
}

/**
* Finds a resolved export that corresponds to the export IR.
* @param exportIr Export IR that is being resolved
* @param bindingsMap Bindings map of the module that contains the export IR
* @return null if no resolved export was found, otherwise the resolved export
*/
private BindingsMap.ExportedModule findResolvedExportForIr(Export exportIr, BindingsMap bindingsMap) {
switch (exportIr) {
case Export.Module exportedModIr -> {
var exportedModName = exportedModIr.name().name();
var foundResolvedExp = bindingsMap.resolvedExports().find(resolvedExport -> {
var resolvedExportName = resolvedExport.target().qualifiedName();
return resolvedExportName.toString().equals(exportedModName);
});
return foundResolvedExp.isEmpty() ? null : foundResolvedExp.get();
}
default -> throw new IllegalStateException("Unexpected value: " + exportIr);
}
}

@Override
public <T extends IR> T updateMetadataInDuplicate(T sourceIr, T copyOfIr) {
return IRPass.super.updateMetadataInDuplicate(sourceIr, copyOfIr);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,11 @@
* Inserts errors into imports/exports IRs if the above conditions are violated.
*/
public final class PrivateModuleAnalysis implements IRPass {
private static final PrivateModuleAnalysis singleton = new PrivateModuleAnalysis();
public static final PrivateModuleAnalysis INSTANCE = new PrivateModuleAnalysis();
private UUID uuid;

private PrivateModuleAnalysis() {}

public static PrivateModuleAnalysis getInstance() {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

In most scenarios, I would say so. But here, I wanted to get as closest to scalac as possible - which creates $MODULE$ public static final field for object. Just my preference. I could refactor it to getter once more.

return singleton;
}

@Override
public void org$enso$compiler$pass$IRPass$_setter_$key_$eq(UUID v) {
this.uuid = v;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ class Passes(
LambdaShorthandToLambda,
ImportSymbolAnalysis,
AmbiguousImportsAnalysis,
PrivateModuleAnalysis.getInstance(),
PrivateModuleAnalysis.INSTANCE,
ExportSymbolAnalysis.INSTANCE,
ShadowedPatternFields,
UnreachableMatchBranches,
NestedPatternMatch,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import org.enso.compiler.pass.analyse.{
AliasAnalysis,
AmbiguousImportsAnalysis,
BindingAnalysis,
ExportSymbolAnalysis,
ImportSymbolAnalysis,
PrivateModuleAnalysis
}
Expand Down Expand Up @@ -61,7 +62,8 @@ class PassesTest extends CompilerTest {
LambdaShorthandToLambda,
ImportSymbolAnalysis,
AmbiguousImportsAnalysis,
PrivateModuleAnalysis.getInstance(),
PrivateModuleAnalysis.INSTANCE,
ExportSymbolAnalysis.INSTANCE,
ShadowedPatternFields,
UnreachableMatchBranches,
NestedPatternMatch,
Expand Down
Loading
Loading