search

tweag / nickel

Better configuration for less
https://nickel-lang.org/
MIT License
2.23k stars 85 forks source link

NLS reports same diagnostic message multiple times #1882

Closed uhlajs closed 3 months ago

uhlajs commented 3 months ago

Describe the bug

NLS reports same diagnostic message multiple times when the recursive definitions is present. Related to #1875.

https://github.com/tweag/nickel/assets/13243167/dca92462-5493-4891-aa1a-83765bb27518

To Reproduce

{
  applications = {
    apps | not_exported = applications,
    message = "message",
    blah | String = 5,
  }
}

Expected behavior

The diagnostic message will be reported only once per case.

Environment

NAME="Fedora Linux"
VERSION="39.20240327.0 (Sericea)"
yannham commented 3 months ago

I wonder what's the best way to avoid this case in the recursive setting. I think @jneem mentioned somewhere that it's not as simple as black-holing or marking fields to not re-evaluate them many times (otherwise we would do that instead of having a max evaluation depth, and also because I guess one field might end up having different copies in different merge expressions, and thus with a different environment).

Maybe just compute a cheap hash of diagnostics and de-duplicate them is the easiest way forward.

yannham commented 3 months ago

(Or, put differently, store the diagnostics in an ordered set [or maybe we don't event care about ordering?] with a cheap hasher instead of a vector)

jneem commented 3 months ago

Sort + deduplication is probably the way to go. We do this for some responses, just because it makes the output deterministic and therefore easy to test. But apparently we aren't (yet) doing it for diagnostics.