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.

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

bug(#368): unlint-non-existing-defect in WPA scope #369

Merged
merged 11 commits into from
Feb 28, 2025
3 changes: 3 additions & 0 deletions src/main/java/org/eolang/lints/Lint.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
*
* @param <T> The type of entity to analyze
* @since 0.0.1
* @todo #368:90min Implement lint caching decorator.
* We should cache `Lint.defects()` to cache found defects when calling
* particular lint. Don't forget to add unit tests, and benchmark tests.
*/
public interface Lint<T> {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import org.cactoos.text.TextOf;

/**
* Lints.
* Lint for checking `+unlint` meta to suppress non-existing defects in single program scope.
*
* @since 0.0.40
*/
Expand Down
149 changes: 149 additions & 0 deletions src/main/java/org/eolang/lints/LtUnlintNonExistingDefectWpa.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
/*
* SPDX-FileCopyrightText: Copyright (c) 2016-2025 Objectionary.com
* SPDX-License-Identifier: MIT
*/
package org.eolang.lints;

import com.github.lombrozo.xnav.Xnav;
import com.jcabi.xml.XML;
import java.io.IOException;
import java.util.Collection;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import org.cactoos.io.ResourceOf;
import org.cactoos.list.ListOf;
import org.cactoos.text.IoCheckedText;
import org.cactoos.text.TextOf;

/**
* Lint for checking `+unlint` meta to suppress non-existing defects in WPA scope.
* This lint was not included in {@link LtUnlintNonExistingDefect}, because we need
* to aggregate the XMIR defects using supplied lints. In {@link LtUnlintNonExistingDefect}
* we work with single program scope, while this class works with WPA scope.
*
* @see LtUnlintNonExistingDefect
* @since 0.0.42
*/
final class LtUnlintNonExistingDefectWpa implements Lint<Map<String, XML>> {

/**
* Lints.
*/
private final Iterable<Lint<Map<String, XML>>> lints;

/**
* Ctor.
*
* @param lnts Lints
*/
LtUnlintNonExistingDefectWpa(final Iterable<Lint<Map<String, XML>>> lnts) {
this.lints = lnts;
}

@Override
public String name() {
return "unlint-non-existing-defect";
}

@Override
public Collection<Defect> defects(final Map<String, XML> pkg) {
final Collection<Defect> defects = new LinkedList<>();
final Map<XML, List<String>> existing = this.existingDefects(pkg);
pkg.values().forEach(
xmir -> {
final Xnav xml = new Xnav(xmir.inner());
final List<String> present = existing.get(xmir);
xml.path("/program/metas/meta[head='unlint']/tail")
.map(xnav -> xnav.text().get())
.collect(Collectors.toSet())
.stream()
.filter(unlint -> !present.contains(unlint))
.forEach(
unlint -> xml
.path(
String.format(
"program/metas/meta[head='unlint' and tail='%s']/@line", unlint
)
)
Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel something's still wrong with indentations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxonfjvipon otherwise qulice complains about cascade indentation

Copy link
Member

@maxonfjvipon maxonfjvipon Feb 27, 2025

Choose a reason for hiding this comment

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

@h1alexbel how about this:

.forEach(
    unlint -> xml.path(
        String.format(
            "program/metas/meta[head='unlint' and tail='%s']/@line", unlint
        )
    )
    .map(xnav -> xnav.text().get())
    .collect(Collectors.toList())
    .forEach(
        line -> defects.add(
            new Defect.Default(
                this.name(),
                Severity.WARNING,
                xml.element("program")
                    .attribute("name")
                    .text()
                    .orElse("unknown"),
                Integer.parseInt(line),
                String.format(
                    "Unlinting rule '%s' doesn't make sense, since there are no defects with it",
                    unlint
                )
            )
        )
    )
)

Copy link
Contributor Author

@h1alexbel h1alexbel Feb 27, 2025

Choose a reason for hiding this comment

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

@maxonfjvipon only this one is valid (dedent with xml.path() args):

.forEach(
    unlint -> xml.path(
        String.format(
            "program/metas/meta[head='unlint' and tail='%s']/@line", unlint
        )
        )
        .map(xnav -> xnav.text().get())
        .collect(Collectors.toList())
        .forEach(
            line -> defects.add(
                new Defect.Default(
                    this.name(),
                    Severity.WARNING,
                    xml.element("program")
                        .attribute("name")
                        .text()
                        .orElse("unknown"),
                    Integer.parseInt(line),
                    String.format(
                        "Unlinting rule '%s' doesn't make sense, since there are no defects with it",
                        unlint
                    )
                )
            )
        )
);

Copy link
Member

@maxonfjvipon maxonfjvipon Feb 27, 2025

Choose a reason for hiding this comment

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

@h1alexbel this part looks ugly:

  )
  )

We definitely should find another way. How about this:

.forEach(
    unlint -> xml.path(
            String.format(
                "program/metas/meta[head='unlint' and tail='%s']/@line", unlint
            )
        )
        .map(xnav -> xnav.text().get())

Copy link
Contributor Author

@h1alexbel h1alexbel Feb 27, 2025

Choose a reason for hiding this comment

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

@maxonfjvipon the best we can do is this, I believe (since we cannot inline xml.path due to too wide line):

.forEach(
    unlint -> xml.path(
        String.format(
            "program/metas/meta[head='unlint' and tail='%s']/@line", unlint
        )
        )
        .map(xnav -> xnav.text().get())
        .collect(Collectors.toList())
        .forEach(
            line -> defects.add(
                new Defect.Default(
                    this.name(),
                    Severity.WARNING,
                    xml.element("program")
                        .attribute("name")
                        .text()
                        .orElse("unknown"),
                    Integer.parseInt(line),
                    String.format(
                        "Unlinting rule '%s' doesn't make sense, since there are no defects with it",
                        unlint
                    )
                )
            )
        )
);

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel what about this?

.forEach(
    unlint -> xml
        .path(
            String.format()
        )
        .map(...)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxonfjvipon I think we did it. Big thanks!

.map(xnav -> xnav.text().get())
.collect(Collectors.toList())
.forEach(
line -> defects.add(
new Defect.Default(
this.name(),
Severity.WARNING,
xml.element("program")
.attribute("name")
.text()
.orElse("unknown"),
Integer.parseInt(line),
String.format(
"Unlinting rule '%s' doesn't make sense, since there are no defects with it",
unlint
)
)
)
)
);
}
);
return defects;
}

@Override
public String motive() throws IOException {
return new IoCheckedText(
new TextOf(
new ResourceOf(
String.format(
"org/eolang/motives/misc/%s.md", this.name()
)
)
)
).asString();
}

/**
* Find existing defects.
*
* @param pkg Package with programs to scan
* @return Map of existing defects
*/
private Map<XML, List<String>> existingDefects(final Map<String, XML> pkg) {
final Map<XML, List<String>> aggregated = new HashMap<>(0);
this.lints.forEach(
wpl -> {
try {
final Collection<Defect> defects = wpl.defects(pkg);
Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel can this line be move out of try catch scope? It seems that .defects does not throw IOException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxonfjvipon Lint#defects() throws IOException:

Collection<Defect> defects(T entity) throws IOException;

Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel it also seems that during the entire project linting we call Lint.defect a several times and on each particular lint. And every time these defects are recalculated. Maybe we should introduce some kind of cache or wrap these lints with cache decorator? Or at least add todo?

pkg.values().forEach(
program ->
aggregated.merge(
program,
new ListOf<>(
defects.stream()
.map(Defect::rule)
.collect(Collectors.toList())
),
(existing, incoming) -> {
existing.addAll(incoming);
return existing;
}
)
);
} catch (final IOException exception) {
throw new IllegalStateException(
String.format(
"IO operation failed while linting package of programs with %s",
wpl.name()
),
exception
);
}
}
);
return aggregated;
}
}
27 changes: 19 additions & 8 deletions src/main/java/org/eolang/lints/PkWpa.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.jcabi.xml.XML;
import java.util.Map;
import org.cactoos.iterable.IterableEnvelope;
import org.cactoos.iterable.Joined;
import org.cactoos.iterable.Mapped;
import org.cactoos.iterable.Shuffled;
import org.cactoos.list.ListOf;
Expand All @@ -21,20 +22,30 @@
*/
final class PkWpa extends IterableEnvelope<Lint<Map<String, XML>>> {

/**
* WPA lints.
*/
private static final Iterable<Lint<Map<String, XML>>> WPA = new ListOf<>(
new LtUnitTestMissing(),
new LtUnitTestWithoutLiveFile(),
new LtIncorrectAlias(),
new LtObjectIsNotUnique(),
new LtAtomIsNotUnique()
);

/**
* Ctor.
*/
PkWpa() {
super(
new Shuffled<>(
new Mapped<>(
lnt -> new LtWpaUnlint(lnt),
new ListOf<>(
new LtUnitTestMissing(),
new LtUnitTestWithoutLiveFile(),
new LtIncorrectAlias(),
new LtObjectIsNotUnique(),
new LtAtomIsNotUnique()
new Mapped<Lint<Map<String, XML>>>(
LtWpaUnlint::new,
new Joined<Lint<Map<String, XML>>>(
PkWpa.WPA,
new ListOf<>(
new LtUnlintNonExistingDefectWpa(PkWpa.WPA)
)
)
)
)
Expand Down
119 changes: 119 additions & 0 deletions src/test/java/org/eolang/lints/LtUnlintNonExistingDefectWpaTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/*
* SPDX-FileCopyrightText: Copyright (c) 2016-2025 Objectionary.com
* SPDX-License-Identifier: MIT
*/
package org.eolang.lints;

import com.jcabi.xml.XML;
import com.jcabi.xml.XMLDocument;
import java.io.IOException;
import org.cactoos.list.ListOf;
import org.cactoos.map.MapEntry;
import org.cactoos.map.MapOf;
import org.eolang.parser.EoSyntax;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Test;

/**
* Tests for {@link LtUnlintNonExistingDefectWpa}.
*
* @since 0.0.42
*/
final class LtUnlintNonExistingDefectWpaTest {

@Test
void allowsUnlintingExistingWpaDefects() throws IOException {
MatcherAssert.assertThat(
"Lint should not complain, since program has WPA defects",
new LtUnlintNonExistingDefectWpa(
new ListOf<>(new LtUnitTestMissing())
).defects(
new MapOf<>(
"foo",
new EoSyntax(
String.join(
"\n",
"+unlint unit-test-missing",
"",
"# Foo",
"[] > foo"
)
).parsed()
)
),
Matchers.emptyIterable()
);
}

@Test
void catchesUnlintOfNonExistingWpaDefects() throws IOException {
MatcherAssert.assertThat(
"Defects are not empty, but they should be, since +unlint unlints non-existing defect",
new LtUnlintNonExistingDefectWpa(
new ListOf<>(new LtUnitTestMissing())
).defects(
new MapOf<String, XML>(
new MapEntry<>(
"bar",
new EoSyntax(
String.join(
"\n",
"+unlint unit-test-missing",
"",
"# Bar",
"[] > bar"
)
).parsed()
),
new MapEntry<>("bar-tests", new XMLDocument("<program/>"))
)
),
Matchers.hasSize(Matchers.greaterThan(0))
);
}

@Test
void allowsNoUnlints() {
MatcherAssert.assertThat(
"Defects are not empty, but they should",
new LtUnlintNonExistingDefectWpa(
new ListOf<>(new LtUnitTestMissing())
).defects(
new MapOf<String, XML>(
new MapEntry<>("f", new XMLDocument("<program/>")),
new MapEntry<>("f-tests", new XMLDocument("<program/>"))
)
),
Matchers.emptyIterable()
);
}

@Test
void reportsWithWpaSupplied() throws IOException {
MatcherAssert.assertThat(
"Defects are empty, but they should not",
new LtUnlintNonExistingDefectWpa(
new PkWpa()
).defects(
new MapOf<String, XML>(
new MapEntry<>(
"e-tests",
new EoSyntax(
"e-tests",
String.join(
"\n",
"+unlint unit-test-without-live-file",
"",
"# E tests.",
"[] > runs-e"
)
).parsed()
),
new MapEntry<>("e", new XMLDocument("<program/>"))
)
),
Matchers.hasSize(Matchers.greaterThan(0))
);
}
}
1 change: 1 addition & 0 deletions src/test/java/org/eolang/lints/ProgramsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ void skipsAllWarnings(@Mktmp final Path dir) throws IOException {
String.join(
"\n",
"+unlint unit-test-missing",
"",
"# Test.",
"[] > foo"
)
Expand Down
Loading