Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Fix some warnings in Clutz. #956

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix some warnings in Clutz. #956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
226 changes: 112 additions & 114 deletions src/main/java/com/google/javascript/clutz/AliasMapBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,114 +1,112 @@
package com.google.javascript.clutz;

import com.google.javascript.rhino.Node;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;

/**
* Closure doesn't handle alias reexports like the following well:
*
* <p>`goog.module('bare.reexport'); const {Class} = goog.require('original.module'); exports =
* Class;`
*
* <p>It completely ignores the goog.require() and gives Class the unknown {?} type.
*
* <p>AliasMapBuilder looks for goog.require statements and generates mappings from the exported
* name of Class (here module$exports$bare$reexport) to the name of the imported symbol (here
* module$exports$original$module.Class), so if clutz finds a symbol that has no type, it can emit
* it as an alias to the correct symbol.
*/
public class AliasMapBuilder extends ImportBasedMapBuilder {
@Override
protected Map<String, String> build(
String localModuleId, Node moduleBody, Set<String> googProvides) {
Map<String, String> aliasMap = new HashMap<>();
if (localModuleId == null) {
//TODO(lucassloan): handle goog.module.get()
return aliasMap;
}

// Loop over the statements, looking for import statements, and build a map from the local variable
// names to the original symbol name eg `const C = goog.require('a.b.c');` will result in the map
// containing 'C' -> 'module$exports$a$b$c'
Map<String, String> localVariableToImportedSymbolNameMap = new HashMap<>();
for (Node statement : moduleBody.children()) {
if (isImportAssignment(statement)) {
// `const C = goog.require()` or
// `const C = goog.module.get()`
String importedModuleId = statement.getFirstFirstChild().getChildAtIndex(1).getString();
String localVariableName = statement.getFirstChild().getString();

String importedSymbolName = buildWholeModuleExportSymbolName(importedModuleId);
localVariableToImportedSymbolNameMap.put(localVariableName, importedSymbolName);
} else if (isImportDestructuringAssignment(statement)) {
// `const {C, Clazz: RenamedClazz} = goog.require()` or
// `const {C, Clazz: RenamedClazz} = goog.module.get()`
String importedModuleId =
statement.getFirstChild().getChildAtIndex(1).getChildAtIndex(1).getString();
for (Node destructured : statement.getFirstFirstChild().children()) {
String originalName = destructured.getString();
// Destructuring can use the original name `const {A} = goog.require("foo.a")` or rename
// it `const {A: RenamedA} = ...`, and closure uses whichever in the symbol name it
// generates, so we have to extract it.
String localVariableName;
if (destructured.getFirstChild() != null) {
// Renaming
localVariableName = destructured.getFirstChild().getString();
} else {
// No rename
localVariableName = originalName;
}

String importedSymbolName = buildNamedExportSymbolName(importedModuleId, originalName);
localVariableToImportedSymbolNameMap.put(localVariableName, importedSymbolName);
}
}
}

// Loop over the statements, looking for export statements, and add mappings to the alias map
// if the export is of a variable that was imported
for (Node statement : moduleBody.children()) {
if (isWholeModuleExportAssignment(statement)) {
// `exports = foo`
String localVariableName = getExportsAssignmentRHS(statement);

if (localVariableToImportedSymbolNameMap.containsKey(localVariableName)) {
aliasMap.put(
buildWholeModuleExportSymbolName(localModuleId),
localVariableToImportedSymbolNameMap.get(localVariableName));
}
} else if (isNamedExportAssignment(statement)) {
// `exports.foo = foo;`
String localVariableName = getExportsAssignmentRHS(statement);
String exportName = getNamedExportName(statement);

if (localVariableToImportedSymbolNameMap.containsKey(localVariableName)) {
aliasMap.put(
buildNamedExportSymbolName(localModuleId, exportName),
localVariableToImportedSymbolNameMap.get(localVariableName));
}
} else if (isNamedExportPropAssignment(statement)) {
// `exports.foo = foo.bar;`
String localVariableName = getExportsAssignmentPropRootName(statement);
if (localVariableName.equals("exports")) {
// This is a "local" alias between two exports from the same module.
// There is no need to for clutz to special handle this, as the JS
// Compiler will resolve this properly.
continue;
}
String localPropName = getExportsAssignmentPropName(statement);
String exportName = getNamedExportName(statement);
String localNamespaceName =
localVariableToImportedSymbolNameMap.containsKey(localVariableName)
? localVariableToImportedSymbolNameMap.get(localVariableName)
: localVariableName;
aliasMap.put(
buildNamedExportSymbolName(localModuleId, exportName),
localNamespaceName + "." + localPropName);
}
}

return aliasMap;
}
}
package com.google.javascript.clutz;

import com.google.javascript.rhino.Node;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;

/**
* Closure doesn't handle alias reexports like the following well:
*
* <p>`goog.module('bare.reexport'); const {Class} = goog.require('original.module'); exports =
* Class;`
*
* <p>It completely ignores the goog.require() and gives Class the unknown {?} type.
*
* <p>AliasMapBuilder looks for goog.require statements and generates mappings from the exported
* name of Class (here module$exports$bare$reexport) to the name of the imported symbol (here
* module$exports$original$module.Class), so if clutz finds a symbol that has no type, it can emit
* it as an alias to the correct symbol.
*/
public class AliasMapBuilder extends ImportBasedMapBuilder {
@Override
protected Map<String, String> build(
String localModuleId, Node moduleBody, Set<String> googProvides) {
Map<String, String> aliasMap = new HashMap<>();
if (localModuleId == null) {
// TODO(lucassloan): handle goog.module.get()
return aliasMap;
}

// Loop over the statements, looking for import statements, and build a map from the local variable
// names to the original symbol name eg `const C = goog.require('a.b.c');` will result in the map
// containing 'C' -> 'module$exports$a$b$c'
Map<String, String> localVariableToImportedSymbolNameMap = new HashMap<>();
for (Node statement : moduleBody.children()) {
if (isImportAssignment(statement)) {
// `const C = goog.require()` or
// `const C = goog.module.get()`
String importedModuleId = statement.getFirstFirstChild().getSecondChild().getString();
String localVariableName = statement.getFirstChild().getString();

String importedSymbolName = buildWholeModuleExportSymbolName(importedModuleId);
localVariableToImportedSymbolNameMap.put(localVariableName, importedSymbolName);
} else if (isImportDestructuringAssignment(statement)) {
// `const {C, Clazz: RenamedClazz} = goog.require()` or
// `const {C, Clazz: RenamedClazz} = goog.module.get()`
String importedModuleId =
statement.getFirstChild().getChildAtIndex(1).getSecondChild().getString();
for (Node destructured : statement.getFirstFirstChild().children()) {
String originalName = destructured.getString();
// Destructuring can use the original name `const {A} = goog.require("foo.a")` or rename
// it `const {A: RenamedA} = ...`, and closure uses whichever in the symbol name it
// generates, so we have to extract it.
String localVariableName;
if (destructured.hasChildren()) {
// Renaming
localVariableName = destructured.getFirstChild().getString();
} else {
// No rename
localVariableName = originalName;
}

String importedSymbolName = buildNamedExportSymbolName(importedModuleId, originalName);
localVariableToImportedSymbolNameMap.put(localVariableName, importedSymbolName);
}
}
}

// Loop over the statements, looking for export statements, and add mappings to the alias map
// if the export is of a variable that was imported
for (Node statement : moduleBody.children()) {
if (isWholeModuleExportAssignment(statement)) {
// `exports = foo`
String localVariableName = getExportsAssignmentRHS(statement);

if (localVariableToImportedSymbolNameMap.containsKey(localVariableName)) {
aliasMap.put(
buildWholeModuleExportSymbolName(localModuleId),
localVariableToImportedSymbolNameMap.get(localVariableName));
}
} else if (isNamedExportAssignment(statement)) {
// `exports.foo = foo;`
String localVariableName = getExportsAssignmentRHS(statement);
String exportName = getNamedExportName(statement);

if (localVariableToImportedSymbolNameMap.containsKey(localVariableName)) {
aliasMap.put(
buildNamedExportSymbolName(localModuleId, exportName),
localVariableToImportedSymbolNameMap.get(localVariableName));
}
} else if (isNamedExportPropAssignment(statement)) {
// `exports.foo = foo.bar;`
String localVariableName = getExportsAssignmentPropRootName(statement);
if (localVariableName.equals("exports")) {
// This is a "local" alias between two exports from the same module.
// There is no need to for clutz to special handle this, as the JS
// Compiler will resolve this properly.
continue;
}
String localPropName = getExportsAssignmentPropName(statement);
String exportName = getNamedExportName(statement);
String localNamespaceName =
localVariableToImportedSymbolNameMap.getOrDefault(localVariableName, localVariableName);
aliasMap.put(
buildNamedExportSymbolName(localModuleId, exportName),
localNamespaceName + "." + localPropName);
}
}

return aliasMap;
}
}
41 changes: 11 additions & 30 deletions src/main/java/com/google/javascript/clutz/DeclarationGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.Iterables.transform;
import static com.google.javascript.rhino.jstype.JSTypeNative.ALL_TYPE;
import static com.google.javascript.rhino.jstype.JSTypeNative.ARRAY_TYPE;
import static com.google.javascript.rhino.jstype.JSTypeNative.OBJECT_TYPE;
import static com.google.javascript.rhino.jstype.JSTypeNative.STRING_TYPE;
import static java.nio.charset.StandardCharsets.UTF_8;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static org.apache.commons.text.StringEscapeUtils.escapeEcmaScript;

import com.google.common.base.CharMatcher;
Expand Down Expand Up @@ -358,8 +358,7 @@ void generateDeclarations() {
continue;
}

getJsEntryPathsFromZip(source)
.stream()
getJsEntryPathsFromZip(source).stream()
.map(p -> SourceFile.fromPath(p, UTF_8))
.forEach(sourceFiles::add);
}
Expand Down Expand Up @@ -392,8 +391,7 @@ void generateDeclarations() {
*/
static List<Path> getJsEntryPathsFromZip(String source) {
try (ZipFile zipFile = new ZipFile(source)) {
return zipFile
.stream()
return zipFile.stream()
.filter(e -> !e.isDirectory())
.filter(e -> e.getName().endsWith(".js"))
.map(e -> source + "!/" + e.getName())
Expand Down Expand Up @@ -1022,13 +1020,6 @@ private String normalizeWindowGlobals(String name) {
return name.replaceAll("^(window|this)\\.", "");
}

/** See the comment above on shouldAvoidGeneratingExterns. */
private boolean shouldAvoidGeneratingExterns(ObjectType type) {
if (type.getConstructor() == null || type.getConstructor().getSource() == null) return false;
return shouldAvoidGeneratingExterns(
type.getConstructor().getSource().getSourceFileName(), type.getDisplayName());
}

private void declareNamespace(
String namespace,
TypedVar symbol,
Expand Down Expand Up @@ -1776,8 +1767,7 @@ private void visitClassOrInterface(String name, FunctionType ftype) {
ObjectType superType = getSuperType(ftype);
if (superType != null) {
emit("extends");
boolean emitInstanceForObject = !shouldAvoidGeneratingExterns(superType);
Visitor<Void> visitor = new ExtendsImplementsTypeVisitor(emitInstanceForObject);
Visitor<Void> visitor = new ExtendsImplementsTypeVisitor();
superType.visit(visitor);
}

Expand All @@ -1797,7 +1787,7 @@ private void emitCommaSeparatedInterfaces(Iterator<ObjectType> it) {
// TypeScript does not allow public APIs that expose non-exported/private types.
emit(getGlobalSymbolNamespacePrefix() + "PrivateInterface");
} else {
ExtendsImplementsTypeVisitor visitor = new ExtendsImplementsTypeVisitor(false);
ExtendsImplementsTypeVisitor visitor = new ExtendsImplementsTypeVisitor();
type.visit(visitor);
}
if (it.hasNext()) {
Expand Down Expand Up @@ -2225,7 +2215,7 @@ public Void caseSymbolType() {

@Override
public Void caseObjectType(ObjectType type) {
return emitObjectType(type, false, false);
return emitObjectType(type, false);
}

@Override
Expand Down Expand Up @@ -2775,7 +2765,8 @@ private void maybeEmitSymbolIterator(JSType instanceType) {
// Unfortunately, this method of detecting whether a class implements Iterator, is error-prone
// in cases where the extension goes through another extends clause. Consider, C extends D,
// and D implements Iterable.
// In such cases C doesn't need to emit anything new, because D's emit would have handled that.
// In such cases C doesn't need to emit anything new, because D's emit would have handled
// that.
// Here we detect one such case - D being the always-present Array type and skip unnecessary
// custom emit.
if (instanceType.isSubtype(arrayType)) {
Expand Down Expand Up @@ -3155,10 +3146,7 @@ private List<String> getTemplateTypeNames(ObjectType objType) {
if (objType.getTemplateTypeMap() == null) {
return Collections.emptyList();
}
return objType
.getTemplateTypeMap()
.getTemplateKeys()
.stream()
return objType.getTemplateTypeMap().getTemplateKeys().stream()
.map(jsType -> jsType != null ? jsType.getDisplayName() : "")
.collect(toImmutableList());
}
Expand Down Expand Up @@ -3463,8 +3451,7 @@ public void emitPrivateValue(String emitName) {
emitBreak();
}

public Void emitObjectType(
ObjectType type, boolean extendingInstanceClass, boolean inExtendsImplementsPosition) {
public Void emitObjectType(ObjectType type, boolean inExtendsImplementsPosition) {
// Closure doesn't require that all the type params be declared, but TS does
if (!type.getTemplateTypeMap().isEmpty()
&& !typeRegistry.getNativeType(OBJECT_TYPE).equals(type)) {
Expand Down Expand Up @@ -3526,15 +3513,9 @@ private String maybeRenameGlobalType(String name) {
* any' is invalid, even though () => any is a valid type.
*/
class ExtendsImplementsTypeVisitor implements Visitor<Void> {
final boolean emitInstanceForObject;

ExtendsImplementsTypeVisitor(boolean emitInstanceForObject) {
this.emitInstanceForObject = emitInstanceForObject;
}

@Override
public Void caseObjectType(ObjectType type) {
emitObjectType(type, emitInstanceForObject, true);
emitObjectType(type, true);
return null;
}

Expand Down
11 changes: 6 additions & 5 deletions src/main/java/com/google/javascript/gents/Options.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package com.google.javascript.gents;

import static com.google.common.base.Charsets.UTF_8;

import com.google.common.collect.ImmutableMap;
import com.google.gson.Gson;
import com.google.gson.reflect.TypeToken;
Expand All @@ -13,6 +11,7 @@
import com.google.javascript.jscomp.parsing.Config;
import java.io.IOException;
import java.lang.reflect.Type;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.ArrayList;
Expand Down Expand Up @@ -158,7 +157,8 @@ private Map<String, String> getExternsMap() throws IOException {
/* empty */
}.getType();
try (JsonReader reader =
new JsonReader(Files.newBufferedReader(Paths.get(externsMapFile), UTF_8))) {
new JsonReader(
Files.newBufferedReader(Paths.get(externsMapFile), StandardCharsets.UTF_8))) {
return new Gson().fromJson(reader, mapType);
}
} else {
Expand All @@ -183,7 +183,7 @@ private Map<String, String> getExternsMap() throws IOException {

if (sourcesManifest != null) {
try {
filesToConvert = Files.readAllLines(Paths.get(sourcesManifest), UTF_8);
filesToConvert = Files.readAllLines(Paths.get(sourcesManifest), StandardCharsets.UTF_8);
} catch (IOException e) {
throw new CmdLineException(
parser, "sources manifest file " + sourcesManifest + " not found.", e);
Expand All @@ -194,7 +194,8 @@ private Map<String, String> getExternsMap() throws IOException {
srcFiles.addAll(arguments);
} else {
try {
srcFiles.addAll(Files.readAllLines(Paths.get(dependenciesManifest), UTF_8));
srcFiles.addAll(
Files.readAllLines(Paths.get(dependenciesManifest), StandardCharsets.UTF_8));
} catch (IOException e) {
throw new CmdLineException(
parser, "dependencies manifest file " + dependenciesManifest + " not found.", e);
Expand Down
Loading