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

Improved reported error for PP1003 #2305

Closed
wants to merge 4 commits into from
Closed

Improved reported error for PP1003 #2305

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ce8281
Improved reported error for PP1003
cblappert Jul 20, 2018
c268db4
Fix wording
cblappert Jul 21, 2018
592a3ea
Fix flow
cblappert Jul 21, 2018
75b4229
Moved getDebugName out
cblappert Jul 24, 2018
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
50 changes: 43 additions & 7 deletions src/serializer/functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import { handleReportedSideEffect } from "./utils.js";
import { ShapeInformation } from "../utils/ShapeInformation";
import type { ArgModel } from "../utils/ShapeInformation";
import * as t from "@babel/types";
import { stringOfLocation } from "../utils/babelhelpers";

type AdditionalFunctionEntry = {
value: ECMAScriptSourceFunctionValue | AbstractValue,
Expand Down Expand Up @@ -299,7 +300,12 @@ export class Functions {
let conflicts: Map<BabelNodeSourceLocation, CompilerDiagnostic> = new Map();
for (let fun1 of additionalFunctions) {
invariant(fun1 instanceof FunctionValue);
let fun1Name = this.functionExpressions.get(fun1) || fun1.intrinsicName || "(unknown function)";
let fun1Location = fun1.expressionLocation;
let fun1Name =
this.functionExpressions.get(fun1) ||
fun1.intrinsicName ||
fun1.__originalName ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than doing intrinsicName || __originalName every time, we should create a helper function like have with React, getFunctionName(). Functions might also have a name as a property, and neither of these ways will get it, so it's best if we start making the name getting part generic in the code base.

`(unknown function ${fun1Location ? stringOfLocation(fun1Location) : ""})`;
// Also do argument validation here
let additionalFunctionEffects = this.writeEffects.get(fun1);
invariant(additionalFunctionEffects !== undefined);
Expand All @@ -318,8 +324,14 @@ export class Functions {
for (let fun2 of additionalFunctions) {
if (fun1 === fun2) continue;
invariant(fun2 instanceof FunctionValue);
let fun2Location = fun2.expressionLocation;
let fun2Name =
this.functionExpressions.get(fun2) ||
fun2.intrinsicName ||
fun2.__originalName ||
`(unknown function ${fun2Location ? stringOfLocation(fun2Location) : ""})`;
let reportFn = () => {
this.reportWriteConflicts(fun1Name, conflicts, e1.modifiedProperties, this._callOfFunction(fun2));
this.reportWriteConflicts(fun1Name, fun2Name, conflicts, e1.modifiedProperties, this._callOfFunction(fun2));
return null;
};
let fun2Effects = this.writeEffects.get(fun2);
Expand All @@ -344,14 +356,25 @@ export class Functions {
}

reportWriteConflicts(
fname: string,
f1name: string,
f2name: string,
conflicts: Map<BabelNodeSourceLocation, CompilerDiagnostic>,
pbs: PropertyBindings,
call2: void => Value
): void {
let reportConflict = (location: BabelNodeSourceLocation) => {
let reportConflict = (
location: BabelNodeSourceLocation,
object: string = "",
key?: string,
originalLocation: BabelNodeSourceLocation | void | null
) => {
let firstLocationString = originalLocation ? ` ${stringOfLocation(originalLocation)}` : "";
let propString = key ? `${object ? "" : "<unknown>"}.${key} ` : "";
let objectString = ` ${object}${propString}`;
let error = new CompilerDiagnostic(
`Property access conflicts with write in optimized function ${fname}`,
`Property modification${objectString}at${firstLocationString} in "${f1name}" conflicts with access at ${
location.start.line
}:${location.start.column} in optimized function "${f2name}"`,
location,
"PP1003",
"FatalError"
Expand All @@ -366,14 +389,27 @@ export class Functions {
this.realm.reportObjectGetOwnProperties = (ob: ObjectValue) => {
let location = this.realm.currentLocation;
invariant(location);
if (writtenObjects.has(ob) && !conflicts.has(location)) reportConflict(location);
if (writtenObjects.has(ob) && !conflicts.has(location))
reportConflict(location, ob.intrinsicName || ob.__originalName, undefined, ob.expressionLocation);
};
let oldReportPropertyAccess = this.realm.reportPropertyAccess;
this.realm.reportPropertyAccess = (pb: PropertyBinding) => {
if (ObjectValue.refuseSerializationOnPropertyBinding(pb)) return;
let location = this.realm.currentLocation;
if (!location) return; // happens only when accessing an additional function property
if (pbs.has(pb) && !conflicts.has(location)) reportConflict(location);
if (pbs.has(pb) && !conflicts.has(location)) {
let originalLocation =
pb.descriptor && pb.descriptor.value && !Array.isArray(pb.descriptor.value)
? pb.descriptor.value.expressionLocation
: undefined;
let keyString = pb.key instanceof Value ? pb.key.toDisplayString() : pb.key;
reportConflict(
location,
pb.object ? pb.object.intrinsicName || pb.object.__originalName : undefined,
keyString,
originalLocation
);
}
};
try {
ignoreErrorsIn(this.realm, () => this.realm.evaluateForEffectsInGlobalEnv(call2));
Expand Down