[core] Add operations to manipulate a document #828

gibarsin · 2018-01-03T14:49:19Z

Summary

To be able to make the fixes persist on the file on which the AST was created on, a new handler is needed that is able to manage all the operations (insert, delete and replace) and apply them in order to the original file.

Additions

Document interface to represent a file in which fixes can be applied.
DocumentOperation represents one operation (insert, delete or replace) that will be managed by the DocumentOperationsApplier as to when or in what order to apply all of them.
RegionByLine and RegionByOffset are both representations of regions in a document. The difference between them are, when one is used over the other. The RegionByLine is first used when an operation is created because the regions of the tokens are represented in this manner and there is no way to map it to (offset, length) until the operation has reached the document, which internally holds a map <LineNumber, Offset> to convert this type of region to a RegionByOffset.

Additional notes

DocumentFile which is an implementation of Document uses a reader and a writer to apply the fixes onto the file. The file is read only once, which improves efficiency, but it is expected to receive all the operations sorted.

… IDEA

gibarsin · 2018-01-03T15:10:31Z

I cannot seem to be able to use lambdas since 1.8 or some API methods since 1.9 because the source compiling version is 1.7, is this done on purpose?

oowekyala · 2018-01-04T01:01:03Z

@gibarsin Yes, most of pmd is compiled with Java 7, except the UI module, and the Apex and Scala modules IIRC. You don't seem to be using many lambdas anyway, it should be simple enough to fix?

gibarsin · 2018-01-04T02:47:49Z

@oowekyala Everything's Java 7 compliant now, thanks for the answer!

jsotuyod · 2018-01-08T00:44:36Z

pmd-core/src/main/java/net/sourceforge/pmd/util/document/DocumentFile.java

+    private int currentPosition = 0;
+
+    private final Path temporaryPath = Files.createTempFile("pmd-", null);
+    private final Writer writer = new FileWriter(temporaryPath.toFile());


prefer Files.newBufferedWriter to avoid finalizers. Refers to AvoidFileStream

I can't use this method because it's documented since Java 1.8

Present in Java 1.7: https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#newBufferedWriter(java.nio.file.Path,%20java.nio.charset.Charset,%20java.nio.file.OpenOption...)

Thanks, I was looking at the method without the charset parameter.

jsotuyod · 2018-01-08T00:45:17Z

pmd-core/src/main/java/net/sourceforge/pmd/util/document/DocumentFile.java

+    private final Writer writer = new FileWriter(temporaryPath.toFile());
+
+    public DocumentFile(final File file) throws IOException {
+        reader = new BufferedReader(new FileReader(file));


prefer Files.newBufferedReader to avoid finalizers. Refers to AvoidFileStream

I can't use this method because it's documented since Java 1.8

ditto https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#newBufferedReader(java.nio.file.Path,%20java.nio.charset.Charset)

Thanks, I was looking at the method without the charset parameter.

jsotuyod · 2018-01-08T00:52:59Z

pmd-core/src/main/java/net/sourceforge/pmd/util/document/DocumentFile.java

+
+            while (scanner.hasNextLine()) {
+                lineToOffset.add(currentGlobalOffset);
+                currentGlobalOffset += scanner.nextLine().length();


this offset computation is actually wrong. scanner.nextLine() will ignore new-line characters (which may be 1 or 2 depending on the usage of LF / CR). Your tests should validate this

How do you recommend that I access the private field lineToOffset in a test to assert the stored values? One approach is to make it protected and create a class which extends this one which adds a getter to the field, but I do not know if it's a pleasant way.

You can start by having your tests not use single-line files... And you should check different new lines are honored. Your current implementation for writing lines also breaks that by forcing the usage of the system-defined new line characters.

jsotuyod · 2018-01-08T00:54:22Z

pmd-core/src/main/java/net/sourceforge/pmd/util/document/DocumentFile.java

+        reader.close();
+        writer.close();
+        Files.copy(temporaryPath, Paths.get(filePath), StandardCopyOption.REPLACE_EXISTING);
+        temporaryPath.toFile().delete();


why do this copy + delete rather than simply rename?

jsotuyod · 2018-01-08T01:45:12Z

...ava/net/sourceforge/pmd/util/document/DocumentOperationsApplierForNonOverlappingRegions.java

+            return comparison;
+        }
+
+        private boolean areInsertOperations(final RegionByLine r1, final RegionByLine r2) {


areInsertOperations doesn't check if the operation is an insert? That's weird...

The name of the method is misleading because it checks for two insert operations at the same offset in the file.

nothing guarantees such assumption

An insert operation doesn't take a region of length greater than zero in the file, but it has an offset to where insert it. For both operations to be an insertion at the same offset they have to start at the same line and column and their length representation at the file to be zero (i.e. beginLine == endLine and beginColumn == endColumn). This is what the method is checking.

That's an implementation detail of an insert operation that:

this class shouldn't know

this class can't guarantee will not be true for any other type of operation

By the class you mean the DocumentOperationNonOverlappingRegionsComparator or DocumentOperationsApplierForNonOverlappingRegions?

It is true that you can create a replace/delete operation with the same region as an insert operation, although it would not make any sense in a real case scenario. The underlying question in this method would be: if both start at the same offset, will they bother each other? If both operations don't start at the same offset, then I have to check that one starts after the other one finishes. If both operations start at the same offset and have length zero then they can be put one after the other, because they won't overlap.

For this scenario, neither.

If that's what your method actually checks, that's how this method should be called.

As a solution I can delegate the comparison of operations to the comparison of their regions by using the comparable interface implemented in a region.

jsotuyod · 2018-01-08T02:09:51Z

...ava/net/sourceforge/pmd/util/document/DocumentOperationsApplierForNonOverlappingRegions.java

+
+    public DocumentOperationsApplierForNonOverlappingRegions(final Document document) {
+        this.document = Objects.requireNonNull(document);
+        comparator = new DocumentOperationNonOverlappingRegionsComparator();


there is no reason for this to be an instance field, could be a static final

jsotuyod · 2018-01-08T02:55:15Z

pmd-core/src/test/java/net/sourceforge/pmd/util/document/DocumentFileTest.java

+
+    @After
+    public void deleteTemporaryFiles() {
+        temporaryFile.delete();


TemporaryFolder does this on it's own

…ine separators in the same source file && other minor fixes

gibarsin · 2018-01-08T05:03:09Z

The way in which the length of the line separator is being obtained now works (tested with one source file with different types of line separators) but DocumentFile knows "too much" about what group of the matchResult to query to have the line separation match by the scanner. I think a better approach would be to extend the Scanner class and add the method getLineLengthWithLineSeparator() which is currently being held in the DocumentFile class.

jsotuyod

I wouldn't bother about that right now. You seem to be constantly over-engineering your solutions; and are yet to:

produce a minimal working sample
actually edit an AST during analysis
bind this to an AST edition

There will be plenty of time to refactor once anything proves to be a problem.

jsotuyod · 2018-01-08T05:12:43Z

pmd-core/src/main/java/net/sourceforge/pmd/util/document/DocumentFile.java

-    private final Path temporaryPath = Files.createTempFile("pmd-", null);
-    private final Writer writer = new FileWriter(temporaryPath.toFile());
+    private final Path temporaryPath = Files.createTempFile("pmd-", ".tmp");
+    private final Writer writer = Files.newBufferedWriter(temporaryPath, StandardCharsets.UTF_8);


the charset assumption may be wrong

Is there anywhere I can get the charset for the file i'm editing?

PMD itself takes it from the user input (CLI, Gradle, Maven, etc.), we should probably honor that. Just have both encodings be the same, and taken at the constructor, so you can hook it to the current execution configuration at the proper layer.

jsotuyod · 2018-01-08T05:12:47Z

pmd-core/src/main/java/net/sourceforge/pmd/util/document/DocumentFile.java


    public DocumentFile(final File file) throws IOException {
-        reader = new BufferedReader(new FileReader(file));
+        reader = Files.newBufferedReader(file.toPath(), StandardCharsets.UTF_8);


the charset assumption may be wrong

jsotuyod · 2018-01-08T05:13:35Z

pmd-core/src/main/java/net/sourceforge/pmd/util/document/DocumentFile.java

        this.filePath = file.getAbsolutePath();
        mapLinesToOffsets();
    }

    private void mapLinesToOffsets() throws IOException {
-        try (Scanner scanner = new Scanner(filePath)) {
+        try (Scanner scanner = new Scanner(Paths.get(filePath))) {


why not have filePath be a Path directly?

jsotuyod · 2018-01-08T05:16:37Z

...ava/net/sourceforge/pmd/util/document/DocumentOperationsApplierForNonOverlappingRegions.java

@@ -85,7 +85,7 @@ public int compare(final DocumentOperation o1, final DocumentOperation o2) {
            return comparison;
        }

-        private boolean areInsertOperations(final RegionByLine r1, final RegionByLine r2) {
+        private boolean areInsertOperationsAtTheSameOffset(final RegionByLine r1, final RegionByLine r2) {


we keep not knowing (from these classes perspective) if these are insert operations. You either check the operation type, or you drop that from the name.

jsotuyod · 2018-01-08T05:18:45Z

pmd-core/src/main/java/net/sourceforge/pmd/util/document/DocumentFile.java

@@ -146,4 +163,8 @@ private void writeUntilEOF() throws IOException {
            writer.write(line);
        }
    }
+
+    public List<Integer> getLineToOffset() {


why public? at most it should be package-private if this is test-only

…visibility of getter in DocumentFile

gibarsin · 2018-01-08T14:47:24Z

I have checked all the problems you mentioned and it seems there are none left.

jsotuyod · 2018-01-25T22:58:17Z

Merged! Thanks for the wait.

Bare in mind I moved the document package outside the util, as these are not really utilities but core functionality. Lots of the stuff under util is undergoing deprecation processes anyway (we already killed a lot of code in 6.0.0, we deprecated even more for 7.0.0, including the old designer code in there)

gibarsin · 2018-01-25T23:43:45Z

@jsotuyod Thanks for the heads up! No worries about the waiting time.

gibarsin added 2 commits January 3, 2018 11:30

Add operations to manipulate a document

8f6484c

Remove source/target configuration for maven-compiler-plugin added by…

f09a9b6

… IDEA

Remove Java 8 and 9 language usages

40bb0a0

jsotuyod added this to the 6.1.0 milestone Jan 7, 2018

jsotuyod self-assigned this Jan 8, 2018

jsotuyod changed the title ~~Add operations to manipulate a document~~ [core] Add operations to manipulate a document Jan 8, 2018

jsotuyod requested changes Jan 8, 2018

View reviewed changes

jsotuyod reviewed Jan 8, 2018

View reviewed changes

[WIP] Working fix for <Line, Offset> mapping to support any type of l…

bb8b2cd

…ine separators in the same source file && other minor fixes

jsotuyod reviewed Jan 8, 2018

View reviewed changes

gibarsin added 3 commits January 8, 2018 02:29

Change name in document operations comparator inner method && Change …

000d72d

…visibility of getter in DocumentFile

Change filePath type from String to Path

de09472

Receive Charset of Document in the constructor

40ad978

jsotuyod approved these changes Jan 9, 2018

View reviewed changes

jsotuyod added an:enhancement An improvement on existing features / rules in:pmd-internals Affects PMD's internals labels Jan 9, 2018

MatiasComercio mentioned this pull request Jan 14, 2018

[core] Autofixes feature development #693

Open

14 tasks

oowekyala added in:autofixes Affects the autofixes framework and removed in:pmd-internals Affects PMD's internals labels Jan 23, 2018

jsotuyod merged commit 40ad978 into pmd:master Jan 25, 2018

jsotuyod added a commit that referenced this pull request Jan 25, 2018

Update changelog, refs #828

476d9a6

gibarsin deleted the documentOperations branch January 25, 2018 23:43

[core] Add operations to manipulate a document #828

[core] Add operations to manipulate a document #828

Conversation

gibarsin commented Jan 3, 2018 • edited

Summary

Additions

Additional notes

gibarsin commented Jan 3, 2018 • edited

oowekyala commented Jan 4, 2018

gibarsin commented Jan 4, 2018

jsotuyod Jan 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsotuyod Jan 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsotuyod Jan 8, 2018 • edited

Choose a reason for hiding this comment

gibarsin Jan 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibarsin commented Jan 8, 2018

jsotuyod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibarsin commented Jan 8, 2018

jsotuyod commented Jan 25, 2018 • edited

gibarsin commented Jan 25, 2018

gibarsin commented Jan 3, 2018 •

edited

gibarsin commented Jan 3, 2018 •

edited

jsotuyod Jan 8, 2018 •

edited

jsotuyod Jan 8, 2018 •

edited

jsotuyod Jan 8, 2018 •

edited

gibarsin Jan 8, 2018 •

edited

jsotuyod commented Jan 25, 2018 •

edited