-
Notifications
You must be signed in to change notification settings - Fork 323
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
Infer method calls on known types and warn about nonexistent methods #11399
base: develop
Are you sure you want to change the base?
Changes from all commits
2715738
c1d4cc1
b284ec5
7968622
f21b783
a1bf4f7
28626fa
9b5a87d
f626367
8aeb690
5c61beb
f5ca4af
435cd0e
8e95c78
218efcd
7594894
9a40731
215a91f
f2b3dd7
273687d
ea15ebc
cafc96d
730e3b9
2aa0377
e39f248
da82e47
52ba2dd
5152227
b7d0fef
94d7323
042edfe
7e7b977
44bfa7d
beba64d
8e58c92
167f9e3
5f2c4aa
02e8275
448c8b2
4cdd9da
d03cba3
64cd1b6
5b23120
23a3828
25f4560
4573289
5a4e057
b21f603
c56c347
2143368
e2f30b2
22db8a6
f6b4638
b98be17
b2c75dc
7cfd7c4
fb5517f
da23709
9dc1268
aa134ab
5181827
157787d
0c202e9
44a4fd0
0e932f4
90fd53a
177e53a
6bb65c1
9df01ef
2cb3fb6
57b6208
70cf5de
6daf829
016d3e1
46e42dd
aa4da65
7d655a4
f90a096
6b621f8
6a2289e
baf4a97
34ef68b
6bf74aa
d936281
4918207
5dbc13a
ef3daa0
bce8508
237af09
1d25d08
855b621
68555a5
990f32f
38c9f78
1563397
d9db891
7719e3e
a912ef4
6eed3eb
ef9095a
bff9a84
fb52dcc
dd0b18b
33a7fac
131116d
981a202
9b43442
4b87f76
57e0859
7794eee
60fff64
436b660
383e2ff
f90f4a2
6519fc1
a437c3e
e994d0c
0aacaf9
ae5dd11
fc88957
efadb43
1b5ffc2
1462e12
1913402
eb9988a
eb8f69e
13c75b0
b7690d5
457a63a
e09047d
894dabc
5f6bf2e
113eec3
5be3b82
4a415cc
3e78eb7
8d060ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,8 +37,10 @@ type Default_Comparator | |
## PRIVATE | ||
hash : Number -> Integer | ||
hash x = Default_Comparator.hash_builtin x | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just trying to apply consistent formatting. |
||
## PRIVATE | ||
hash_builtin x = @Builtin_Method "Default_Comparator.hash_builtin" | ||
|
||
## PRIVATE | ||
less_than_builtin left right = @Builtin_Method "Default_Comparator.less_than_builtin" | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,6 +91,9 @@ type Project_Description | |
namespace : Text | ||
namespace self = self.ns | ||
|
||
## PRIVATE | ||
enso_project_builtin module = @Builtin_Method "Project_Description.enso_project_builtin" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @radeusgd has a good point. The corresponding builtin method is already defined as EnsoProjectNode. Thus, this code does not introduce any changes. It seems that BuiltinFunction.isAutoRegister makes it possible to defined a builtin method in Java without any shadow definition in Enso code. I think having these kind of shadow definitions is beneficial, as we can immediately see all the definitions of builtin methods on a type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's how auto register was supposed to work. |
||
|
||
## ICON enso_icon | ||
Returns the Enso project description for the project that the engine was | ||
executed with, i.e., the project that contains the `main` method, or | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -832,6 +832,12 @@ type File | |
to_display_text : Text | ||
to_display_text self = self.to_text | ||
|
||
## PRIVATE | ||
copy_builtin self target options = @Builtin_Method "File.copy_builtin" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seeing all these (undesirable) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are no changes to builtin methods structure. I'm only reproducing in Enso code the structure that has already been there in the builtins. That is - we already were having calls to If that structure is undesirable, we should restructure the builtins, but that is not really in the scope of the PR. But if you will not allow me to merge it without that, I will probably be forced to do this unrelated change.
I've added checking of method calls. What methods are available on an object was taken from the type definitions. If a builtin method was called e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the other hand, these changes are not absolutely required for the type inference changes. They are there to make the tests pass, but I can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like these kind of shadow definitions of builtin methods. They don't (at least they should not) introduce any changes (that is, they don't introduce any new builtins) and they make it easy to see which builtin methods are defined on a type. It may be worth revisiting BuiltinMethod.autoRegister annotation argument. See my other comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see. That's indeed related.
Builtin methods are special case! They are not behaving as normal methods.
OK.
What's the benefit of registering a builtin automatically with the type, @hubertp? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been a while ago but IIRC that was specifically requested either by you or @kustosz, so that we don't have to write all those builtin methods as it is done now in this PR. |
||
|
||
## PRIVATE | ||
move_builtin self target options = @Builtin_Method "File.move_builtin" | ||
|
||
## PRIVATE | ||
|
||
Utility function that returns all descendants of the provided file, including | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,12 +3,13 @@ | |
/** Defines a stage of compilation of the module. */ | ||
public enum CompilationStage { | ||
INITIAL(0), | ||
AFTER_PARSING(1), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the purpose of this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was introducing a new stage: But I can just bring this back to a natural 0, 1, 2... ordering if that's preferred - please let me know. |
||
AFTER_IMPORT_RESOLUTION(2), | ||
AFTER_GLOBAL_TYPES(3), | ||
AFTER_STATIC_PASSES(4), | ||
AFTER_RUNTIME_STUBS(5), | ||
AFTER_CODEGEN(6); | ||
AFTER_PARSING(10), | ||
AFTER_IMPORT_RESOLUTION(20), | ||
AFTER_GLOBAL_TYPES(30), | ||
AFTER_STATIC_PASSES(40), | ||
AFTER_TYPE_INFERENCE_PASSES(45), | ||
AFTER_RUNTIME_STUBS(50), | ||
AFTER_CODEGEN(60); | ||
|
||
private final int ordinal; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,4 +29,5 @@ | |
exports org.enso.compiler.phase; | ||
exports org.enso.compiler.phase.exports; | ||
exports org.enso.compiler.refactoring; | ||
exports org.enso.compiler.common_logic; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No snakecase, please. That's not typical Java convention. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,191 @@ | ||
package org.enso.compiler.common_logic; | ||
|
||
import org.enso.compiler.MetadataInteropHelpers; | ||
import org.enso.compiler.core.ir.Module; | ||
import org.enso.compiler.core.ir.module.scope.Definition; | ||
import org.enso.compiler.core.ir.module.scope.definition.Method; | ||
import org.enso.compiler.core.ir.module.scope.imports.Polyglot; | ||
import org.enso.compiler.data.BindingsMap; | ||
import org.enso.compiler.pass.resolve.MethodDefinitions$; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import scala.jdk.javaapi.CollectionConverters; | ||
|
||
/** | ||
* Gathers the common logic for building the ModuleScope. | ||
* | ||
* <p>This is done in two places: | ||
* | ||
* <ol> | ||
* <li>in the compiler, gathering just the types to build StaticModuleScope, | ||
* <li>in the runtime, building Truffle nodes for the interpreter. | ||
* </ol> | ||
* | ||
* <p>The interpreter does much more than the type-checker, so currently this only gathers the | ||
* general shape of the process to try to ensure that they stay in sync. In future iterations, we | ||
* may try to move more of the logic to this common place. | ||
*/ | ||
public abstract class BuildScopeFromModuleAlgorithm< | ||
FunctionType, | ||
TypeScopeReferenceType, | ||
ImportExportScopeType, | ||
ModuleScopeType extends | ||
CommonModuleScopeShape<FunctionType, TypeScopeReferenceType, ImportExportScopeType>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow! One should always beware of what one asks for!
Showing object algebras to a functional programmers may lead to code that I no longer understand... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there is any need for If you want to perform operation on Avoid There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm, okay I can rewrite it to such a form. It felt to me that it's a bit easier and allows me to separate concerns - the parts providing the If I put these methods directly into the algorithm, I'll need to duplicate them for each one. How can I ensure I can still share them? I guess I need to create a separate class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's indeed my preference. As an alternative...
... you can try to convince me that your design is right. I just feel scared for a signature like:
|
||
ModuleScopeBuilderType extends | ||
CommonModuleScopeShape.Builder< | ||
FunctionType, TypeScopeReferenceType, ImportExportScopeType, ModuleScopeType>> { | ||
private final Logger logger = LoggerFactory.getLogger(BuildScopeFromModuleAlgorithm.class); | ||
|
||
/** The scope builder to which the algorithm will register the entities. */ | ||
protected final ModuleScopeBuilderType scopeBuilder; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this field There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think child classes were also using it in the implementations of the Because alternatively, I could make it |
||
|
||
protected BuildScopeFromModuleAlgorithm(ModuleScopeBuilderType scopeBuilder) { | ||
this.scopeBuilder = scopeBuilder; | ||
} | ||
|
||
/** Runs the main processing on a module, that will build the module scope for it. */ | ||
public void processModule(Module moduleIr, BindingsMap bindingsMap) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall this method be overriden? If not, make it |
||
processModuleExports(bindingsMap); | ||
processModuleImports(bindingsMap); | ||
processPolyglotImports(moduleIr); | ||
|
||
processBindings(moduleIr); | ||
} | ||
|
||
private void processModuleExports(BindingsMap bindingsMap) { | ||
for (var exportedMod : | ||
CollectionConverters.asJavaCollection(bindingsMap.getDirectlyExportedModules())) { | ||
ImportExportScopeType exportScope = buildExportScope(exportedMod); | ||
scopeBuilder.addExport(exportScope); | ||
} | ||
} | ||
|
||
private void processModuleImports(BindingsMap bindingsMap) { | ||
for (var imp : CollectionConverters.asJavaCollection(bindingsMap.resolvedImports())) { | ||
for (var target : CollectionConverters.asJavaCollection(imp.targets())) { | ||
if (target instanceof BindingsMap.ResolvedModule resolvedModule) { | ||
var importScope = buildImportScope(imp, resolvedModule); | ||
scopeBuilder.addImport(importScope); | ||
} | ||
} | ||
} | ||
} | ||
|
||
private void processPolyglotImports(Module moduleIr) { | ||
for (var imp : CollectionConverters.asJavaCollection(moduleIr.imports())) { | ||
if (imp instanceof Polyglot polyglotImport) { | ||
if (polyglotImport.entity() instanceof Polyglot.Java javaEntity) { | ||
processPolyglotJavaImport(polyglotImport.getVisibleName(), javaEntity.getJavaName()); | ||
} else { | ||
throw new IllegalStateException( | ||
"Unsupported polyglot import entity: " + polyglotImport.entity()); | ||
} | ||
} | ||
} | ||
} | ||
|
||
private void processBindings(Module module) { | ||
for (var binding : CollectionConverters.asJavaCollection(module.bindings())) { | ||
switch (binding) { | ||
case Definition.Type typ -> processTypeDefinition(typ); | ||
case Method.Explicit method -> processMethodDefinition(method); | ||
case Method.Conversion conversion -> processConversion(conversion); | ||
default -> logger.warn( | ||
"Unexpected binding type: {}", binding.getClass().getCanonicalName()); | ||
} | ||
} | ||
} | ||
|
||
/** Allows the implementation to specify how to register polyglot Java imports. */ | ||
protected abstract void processPolyglotJavaImport(String visibleName, String javaClassName); | ||
|
||
/** | ||
* Allows the implementation to specify how to register conversions. | ||
* | ||
* <p>In the future we may want to extract some common logic from this, but for now we allow the | ||
* implementation to specify this. | ||
*/ | ||
protected abstract void processConversion(Method.Conversion conversion); | ||
|
||
/** Allows the implementation to specify how to register method definitions. */ | ||
protected abstract void processMethodDefinition(Method.Explicit method); | ||
|
||
/** | ||
* Allows the implementation to specify how to register type definitions, along with their | ||
* constructors and getters. | ||
* | ||
* <p>The type registration (registering constructors, getters) is really complex, ideally we'd | ||
* also like to extract some common logic from it. But the differences are very large, so setting | ||
* that aside for later. | ||
*/ | ||
protected abstract void processTypeDefinition(Definition.Type typ); | ||
|
||
/** | ||
* Common method that allows to extract the type on which the method is defined. | ||
* | ||
* <ul> | ||
* <li>For a member method, this will be its parent type. | ||
* <li>For a static method, this will be the eigentype of the type on which it is defined. | ||
* <li>For a module method, this will be the type associated with the module. | ||
* </ul> | ||
*/ | ||
protected final TypeScopeReferenceType getTypeAssociatedWithMethod(Method.Explicit method) { | ||
var typePointerOpt = method.methodReference().typePointer(); | ||
if (typePointerOpt.isEmpty()) { | ||
return scopeBuilder.getAssociatedType(); | ||
} else { | ||
var metadata = | ||
MetadataInteropHelpers.getMetadataOrNull( | ||
typePointerOpt.get(), MethodDefinitions$.MODULE$, BindingsMap.Resolution.class); | ||
if (metadata == null) { | ||
logger.debug( | ||
"Failed to resolve type pointer for method: {}", method.methodReference().showCode()); | ||
return null; | ||
} | ||
|
||
return switch (metadata.target()) { | ||
case BindingsMap.ResolvedType resolvedType -> associatedTypeFromResolvedType( | ||
resolvedType, method.isStatic()); | ||
case BindingsMap.ResolvedModule resolvedModule -> associatedTypeFromResolvedModule( | ||
resolvedModule); | ||
default -> throw new IllegalStateException( | ||
"Unexpected target type: " + metadata.target().getClass().getCanonicalName()); | ||
}; | ||
} | ||
} | ||
|
||
/** | ||
* Implementation specific piece of {@link #getTypeAssociatedWithMethod(Method.Explicit)} that | ||
* specifies how to build the associated type from a resolved module. | ||
*/ | ||
protected abstract TypeScopeReferenceType associatedTypeFromResolvedModule( | ||
BindingsMap.ResolvedModule module); | ||
|
||
/** | ||
* Implementation specific piece of {@link #getTypeAssociatedWithMethod(Method.Explicit)} that | ||
* specifies how to build the associated type from a resolved type, depending on if the method is | ||
* static or not. | ||
*/ | ||
protected abstract TypeScopeReferenceType associatedTypeFromResolvedType( | ||
BindingsMap.ResolvedType type, boolean isStatic); | ||
|
||
/** | ||
* Allows the implementation to specify how to build the export scope from an exported module | ||
* instance. | ||
* | ||
* <p>Such scope is then registered with the scope builder using {@link | ||
* ModuleScopeBuilderType#addExport}. | ||
*/ | ||
protected abstract ImportExportScopeType buildExportScope( | ||
BindingsMap.ExportedModule exportedModule); | ||
|
||
/** | ||
* Allows the implementation to specify how to build the import scope from a resolved import and | ||
* module. | ||
* | ||
* <p>Such scope is then registered with the scope builder using {@link | ||
* ModuleScopeBuilderType#addImport}. | ||
*/ | ||
protected abstract ImportExportScopeType buildImportScope( | ||
BindingsMap.ResolvedImport resolvedImport, BindingsMap.ResolvedModule resolvedModule); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
package org.enso.compiler.common_logic; | ||
|
||
import java.util.Collection; | ||
|
||
/** | ||
* A common type for module scopes. | ||
* | ||
* <p>It encapsulates the common parts of the module scope that is shared between: | ||
* | ||
* <ul> | ||
* <li>the ModuleScope in the runtime | ||
* <li>the StaticModuleScope in the type checker | ||
* </ul> | ||
* | ||
* @param <FunctionType> the type to which methods are resolved - in the interpreter this will be a | ||
* callable Function reference, in the type-checker this will be a type signature | ||
* @param <TypeScopeReferenceType> the type of type-scope references - they define an 'identity' of | ||
* a type (atom type, its eigen type or module-associated type) | ||
* @param <ImportExportScopeType> the type of import/export scopes - helper types that tie | ||
* import/export relations between modules | ||
*/ | ||
public interface CommonModuleScopeShape< | ||
FunctionType, TypeScopeReferenceType, ImportExportScopeType> { | ||
|
||
/** | ||
* Returns a method with a given name, defined as a method of the provided type, defined in the | ||
* current scope, or {@code null} if not found. | ||
*/ | ||
FunctionType getMethodForType(TypeScopeReferenceType type, String methodName); | ||
|
||
/** Returns the collection containing all imports present in the current module. */ | ||
Collection<ImportExportScopeType> getImports(); | ||
|
||
/** Returns the collection containing all exports present in the current module. */ | ||
Collection<ImportExportScopeType> getExports(); | ||
|
||
FunctionType getConversionFor(TypeScopeReferenceType target, TypeScopeReferenceType source); | ||
|
||
interface Builder< | ||
FunctionType, | ||
TypeScopeReferenceType, | ||
ImportExportScopeType, | ||
ModuleScopeType extends | ||
CommonModuleScopeShape<FunctionType, TypeScopeReferenceType, ImportExportScopeType>> { | ||
|
||
/** | ||
* Builds the module scope and seals it, making it immutable. | ||
* | ||
* <p>The builder should not be used anymore after this method has been called. | ||
*/ | ||
ModuleScopeType build(); | ||
|
||
/** Returns the type (type-scope) associated with the current module. */ | ||
TypeScopeReferenceType getAssociatedType(); | ||
|
||
/** Registers an export in the module scope. */ | ||
void addExport(ImportExportScopeType exportScope); | ||
|
||
/** Registers an import in the module scope. */ | ||
void addImport(ImportExportScopeType importScope); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this method.
@Builtin_Method
s are supposed to be only inprivate
modules. Moreover there already is such aless_than_builtin
inOrdering_Helpers.enso
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this method is indeed wrong, as I don't see a
Comparable.less_than_builtin
builtin in Java.It was probably an artifact from some merges that happened along the way (or just my oversight). I'll remove this one.