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

NLS reports same diagnostic message multiple times #1882

Closed
uhlajs opened this issue Apr 5, 2024 · 3 comments · Fixed by #1883
Closed

NLS reports same diagnostic message multiple times #1882

uhlajs opened this issue Apr 5, 2024 · 3 comments · Fixed by #1883

Comments

@uhlajs
Copy link

uhlajs commented Apr 5, 2024

Describe the bug

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

2024-04-05.08-43-24.mp4

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

  • OS name + version:
NAME="Fedora Linux"
VERSION="39.20240327.0 (Sericea)"
@yannham
Copy link
Member

yannham commented Apr 5, 2024

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
Copy link
Member

yannham commented Apr 5, 2024

(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
Copy link
Member

jneem commented Apr 5, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants