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

chore: Refactor and document CodePathSegment #17474

Merged
merged 3 commits into from Aug 22, 2023
Merged

chore: Refactor and document CodePathSegment #17474

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7ba183
chore: Refactor and document CodePathSegment
nzakas Aug 16, 2023
f0b7a7c
Update lib/linter/code-path-analysis/code-path-segment.js
nzakas Aug 18, 2023
ff5d50f
Remove unneeded array
nzakas Aug 21, 2023
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
71 changes: 51 additions & 20 deletions lib/linter/code-path-analysis/code-path-segment.js
View file
Open in desktop
@@ -1,5 +1,5 @@
/**
* @fileoverview A class of the code path segment.
* @fileoverview The CodePathSegment class.
* @author Toru Nagashima
*/

Expand Down Expand Up @@ -30,10 +30,22 @@ function isReachable(segment) {

/**
* A code path segment.
*
* Each segment is arranged in a series of linked lists (implemented by arrays)
* that keep track of the previous and next segments in a code path. In this way,
* you can navigate between all segments in any code path so long as you have a
* reference to any segment in that code path.
*
* When first created, the segment is in a detached state, meaning that it knows the
* segments that came before it but those segments don't know that this new segment
* follows it. Only when `CodePathSegment#markUsed()` is called on a segment does it
* officially become part of the code path by updating the previous segments to know
* that this new segment follows.
*/
class CodePathSegment {

/**
* Creates a new instance.
* @param {string} id An identifier.
* @param {CodePathSegment[]} allPrevSegments An array of the previous segments.
* This array includes unreachable segments.
Expand All @@ -49,27 +61,25 @@ class CodePathSegment {
this.id = id;

/**
* An array of the next segments.
* An array of the next reachable segments.
* @type {CodePathSegment[]}
*/
this.nextSegments = [];

/**
* An array of the previous segments.
* An array of the previous reachable segments.
* @type {CodePathSegment[]}
*/
this.prevSegments = allPrevSegments.filter(isReachable);

/**
* An array of the next segments.
* This array includes unreachable segments.
* An array of all next segments including reachable and unreachable.
* @type {CodePathSegment[]}
*/
this.allNextSegments = [];

/**
* An array of the previous segments.
* This array includes unreachable segments.
* An array of all previous segments including reachable and unreachable.
* @type {CodePathSegment[]}
*/
this.allPrevSegments = allPrevSegments;
Expand All @@ -83,7 +93,11 @@ class CodePathSegment {
// Internal data.
Object.defineProperty(this, "internal", {
value: {

// determines if the segment has been attached to the code path
used: false,

// array of previous segments coming from the end of a loop
loopedPrevSegments: []
}
});
Expand Down Expand Up @@ -113,9 +127,10 @@ class CodePathSegment {
}

/**
* Creates a segment that follows given segments.
* Creates a new segment that and appends it after the given segments.
nzakas marked this conversation as resolved.
Show resolved Hide resolved
* @param {string} id An identifier.
* @param {CodePathSegment[]} allPrevSegments An array of the previous segments.
* @param {CodePathSegment[]} allPrevSegments An array of the previous segments
* to append to.
* @returns {CodePathSegment} The created segment.
*/
static newNext(id, allPrevSegments) {
Expand All @@ -127,7 +142,7 @@ class CodePathSegment {
}

/**
* Creates an unreachable segment that follows given segments.
* Creates an unreachable segment and appends it after the given segments.
* @param {string} id An identifier.
* @param {CodePathSegment[]} allPrevSegments An array of the previous segments.
* @returns {CodePathSegment} The created segment.
Expand All @@ -137,7 +152,7 @@ class CodePathSegment {

/*
* In `if (a) return a; foo();` case, the unreachable segment preceded by
* the return statement is not used but must not be remove.
* the return statement is not used but must not be removed.
*/
CodePathSegment.markUsed(segment);

Expand All @@ -157,7 +172,7 @@ class CodePathSegment {
}

/**
* Makes a given segment being used.
* Marks a given segment as used.
*
* And this function registers the segment into the previous segments as a next.
* @param {CodePathSegment} segment A segment to mark.
Expand All @@ -172,13 +187,27 @@ class CodePathSegment {
let i;

if (segment.reachable) {

/*
* If the segment is reachable, then it's officially part of the
* code path. This loops through all previous segments to update
* their list of next segments. Because the segment is reachable,
* it's added to both `nextSegments` and `allNextSegments`.
*/
for (i = 0; i < segment.allPrevSegments.length; ++i) {
const prevSegment = segment.allPrevSegments[i];

prevSegment.allNextSegments.push(segment);
prevSegment.nextSegments.push(segment);
}
} else {

/*
* If the segment is not reachable, then it's not officially part of the
* code path. This loops through all previous segments to update
* their list of next segments. Because the segment is not reachable,
* it's added only to `allNextSegments`.
*/
for (i = 0; i < segment.allPrevSegments.length; ++i) {
segment.allPrevSegments[i].allNextSegments.push(segment);
}
Expand All @@ -196,19 +225,21 @@ class CodePathSegment {
}

/**
* Replaces unused segments with the previous segments of each unused segment.
* @param {CodePathSegment[]} segments An array of segments to replace.
* @returns {CodePathSegment[]} The replaced array.
* Creates a new array based on an array of segments. If any segment in the
* array is unused, then it is replaced by all of its previous segments.
* All used segments are returned as-is without replacement.
* @param {CodePathSegment[]} segments The array of segments to flatten.
* @returns {CodePathSegment[]} The flattened array.
*/
static flattenUnusedSegments(segments) {
const done = Object.create(null);
const done = new Set();
const retv = [];
Copy link
Member

Choose a reason for hiding this comment

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

Now that retv merely replicates the contents of done, I think that we could remove it completely and just return the contents of done as an array instead, i.e. (at the end of the function):

return [...done];

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that sets guarantee the order of the items that are added to it, which is why I kept the array.

Copy link
Member

Choose a reason for hiding this comment

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

The order of iteration of elements in a Set is defined to be the same as the order of insertion. This means that [...done] has elements in the same order as they were add()ed to the set - when not already included.

This is explained for example in MDN where it states that Set.prototype[Symbol.iterator]():

Returns a new iterator object that yields the values for each element in the Set object in insertion order.

Ultimately, this behavior is defined by the specification, although in a more technical and abstract language. A Set is specified as having a list (the internal property [[SetData]]) where element are stored in an ordered sequence, so the order of the elements in reading and writing operations is well-defined and does not depend on the implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah nice, thanks for the reference. I'll update the code.


for (let i = 0; i < segments.length; ++i) {
const segment = segments[i];

// Ignores duplicated.
if (done[segment.id]) {
if (done.has(segment)) {
continue;
}

Expand All @@ -217,13 +248,13 @@ class CodePathSegment {
for (let j = 0; j < segment.allPrevSegments.length; ++j) {
const prevSegment = segment.allPrevSegments[j];

if (!done[prevSegment.id]) {
done[prevSegment.id] = true;
if (!done.has(prevSegment)) {
done.add(prevSegment);
retv.push(prevSegment);
}
}
} else {
done[segment.id] = true;
done.add(segment);
retv.push(segment);
}
}
Expand Down