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

Show errors and warnings for the 'smlnj' linter #3957

Merged
merged 2 commits into from
Nov 15, 2021
Merged

Show errors and warnings for the 'smlnj' linter #3957

merged 2 commits into from
Nov 15, 2021

Conversation

cwfoo
Copy link
Contributor

@cwfoo cwfoo commented Oct 26, 2021

Fixes #3953

@cwfoo
Copy link
Contributor Author

cwfoo commented Oct 26, 2021

@hsanson @benknoble Please take a look.

@hsanson
Copy link
Contributor

hsanson commented Oct 26, 2021

Looks good. Appreciated if @benknoble can test this before merging it.

@benknoble
Copy link
Contributor

I will take a look tonight if I can

@benknoble
Copy link
Contributor

benknoble commented Oct 27, 2021

Working on a single file looked fine. Working on a CM-based project worked fine, partly because ALE only uses one or the other (and prefers CM when it can).

Here is a failure I found:

  • Put val b = 1 in b.sml, then open a new file (save or don't) and put use "b.sml"; val a: bool = b. SML reports issues, but I couldn't see them in ALE (they show up as errors for the file = stdIn: the SML repl doesn't handle some kinds of output too well, so there are occasional spurious - and = prefixes).

On a related note, if the error isn't in the file I'm currently looking at, :lopen shows it (or :copen, if you use quickfix). But ale#statusline#Count(bufnr('%')) doesn't report them. Perhaps that changed recently, but I thought it used to be a total count, not just the ones specific to that buffer.

@cwfoo
Copy link
Contributor Author

cwfoo commented Oct 28, 2021

I could change the regex from ^\(- = \)\?stdIn$ to stdIn$ and hope that no one ever uses filenames ending with "stdIn" for their SML source files (which should have a ".sml" extension anyway). What do you think?

@benknoble
Copy link
Contributor

Saw the new commit: did you try it out on the case I mentioned? I haven't yet.

@benknoble
Copy link
Contributor

If @cwfoo can confirm their latest commit fixes the case I mentioned, then I approve.

@cwfoo
Copy link
Contributor Author

cwfoo commented Nov 12, 2021

@benknoble With the latest commit (i.e. "Change smlnj stdIn regex"), ALE would show Error: pattern and expression in val dec don't agree [tycon mismatch] on the first line of a.sml in your a.sml/b.sml example.

(I tried to test this yesterday, but I was bitten by a CM file that I left in my home directory some time ago. ALE really prefers CM!).

@benknoble
Copy link
Contributor

@benknoble With the latest commit (i.e. "Change smlnj stdIn regex"), ALE would show Error: pattern and expression in val dec don't agree [tycon mismatch] on the first line of a.sml in your a.sml/b.sml example.

Great, than I think is ready to merge @hsanson

(I tried to test this yesterday, but I was bitten by a CM file that I left in my home directory some time ago. ALE really prefers CM!).

Well, that's because CM should be preferred :) actually, it's because if there's a CM file, than there's likely no other way to correctly (try to) compile the file with SML/NJ—simply running it may not work if depends on modules declared in the CM file. OTOH, invoking it via CM is guaranteed to work. Granted, the CM file finder is a little… eager… in that it looks upwards and takes the first one it finds (even with multiple in the same directory). If there is a better way to handle this, suggestions are welcome.

Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

@hsanson hsanson merged commit 01fdd8d into dense-analysis:master Nov 15, 2021
@cwfoo cwfoo deleted the fix-sml branch November 15, 2021 12:22
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 this pull request may close these issues.

SML: errors are no longer shown
3 participants