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

Add source file to project parse errors and warnings #10644

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

philderbeast
Copy link
Collaborator

@philderbeast philderbeast commented Dec 16, 2024

Fixes #10635. Improves warning and error messages shown when parsing project files and their imports.

Warning Messages

To trigger these warning messages, the examples use badly formed comments that have a single dash instead of two as is required of a line comment in .cabal and .project files (and imported .config files).

  • Before the fix:

    The cabal.project file name is repeated. Warnings are misattributed to having been in the project rather than from a configuration file imported by the project. Warnings are shown in reverse line number order.

    $ ~/.ghcup/bin/cabal-3.12.1.0 build all --dry-run
    ...
    Warning:
    /.../ParseWarningProvenance/cabal.project,
    cabal.project, cabal.project, cabal.project, cabal.project: Unrecognized
    section '-' on line 123
    /.../ParseWarningProvenance/cabal.project,
    cabal.project, cabal.project, cabal.project, cabal.project: Unrecognized
    section '-' on line 3
    /.../ParseWarningProvenance/cabal.project,
    cabal.project, cabal.project, cabal.project, cabal.project: Unrecognized
    section '-' on line 2
    /.../ParseWarningProvenance/cabal.project,
    cabal.project, cabal.project, cabal.project, cabal.project: Unrecognized
    section '-' on line 1
    /.../ParseWarningProvenance/cabal.project,
    cabal.project, cabal.project, cabal.project, cabal.project: Unrecognized
    section '-' on line 123
    /.../ParseWarningProvenance/cabal.project,
    cabal.project, cabal.project, cabal.project, cabal.project: Unrecognized
    section '-' on line 3
    /.../ParseWarningProvenance/cabal.project,
    cabal.project, cabal.project, cabal.project, cabal.project: Unrecognized
    section '-' on line 2
    /.../ParseWarningProvenance/cabal.project,
    cabal.project, cabal.project, cabal.project, cabal.project: Unrecognized
    section '-' on line 1
    
  • After the fix:

    The warnings are shown in a list. For warnings within the same .project or imported .config file, warnings are sorted by line number. The file that is the source of the warning is shown.

    The warnings associated with configuration files are shown in the order these files were imported by the project:

    $ cat cabal.project
    packages: no-pkg-dir
    import: dir-x/a.config
    import: dir-y/a.config
    import: x.config
    import: y.config
    
    $ cabal build all --dry-run
    ...
    Warnings found while parsing the project file, cabal.project:
    - dir-x/a.config: Unrecognized section '-' on line 1
    - dir-x/a.config: Unrecognized section '-' on line 2
    - dir-x/a.config: Unrecognized section '-' on line 3
    - dir-y/a.config: Unrecognized section '-' on line 123
    - x.config: Unrecognized section '-' on line 1
    - x.config: Unrecognized section '-' on line 2
    - x.config: Unrecognized section '-' on line 3
    - y.config: Unrecognized section '-' on line 123
    

Error Messages from Project

To trigger these error messages, the examples use badly formed conditions:

$ cat cabal.project
-- The following failing condition is not on the first line so we can check the
-- line number:
if _
  • Before the fix:

    The parse error is shown with hard line breaks.

    $ ~/.ghcup/bin/cabal-3.12.1.0 build all --dry-run
    ...
    Error: [Cabal-7090]
    Error parsing project file /.../ParseErrorProvenance/cabal.project:3:
    "<condition>" (line 1, column 1):
    unexpected SecArgName (Position 1 4) "_"
    
  • After the fix:

    The snippet that failed to parse may be shown and the parse error is shown as one line, with no hard line breaks.

    $ cabal build all --dry-run
    ...
    Error: [Cabal-7090]
    Error parsing project file cabal.project:3:
    - Failed to parse 'if(_)' with error:
        "<condition>" (line 1, column 1): unexpected SecArgName (Position 1 4) "_"
    

Error Messages from Imported Config

With the same setup but now with the error in an imported file:

$ cat elif.project 
import: dir-elif/elif.config

$ cat dir-elif/elif.config 
-- The following failing condition is not on the first line so we can check the
-- line number:
if false
elif _
  • Before the fix:

    The project rather than the imported configuration file is shown as the source file.

    $ ~/.ghcup/bin/cabal-3.12.1.0 build all --dry-run
    ...
    Error: [Cabal-7090]
    Error parsing project file /.../ParseErrorProvenance/elif.project:4:
    "<condition>" (line 1, column 1):
    unexpected SecArgName (Position 1 6) "_"
    
  • After the fix:

    The imported configuration file is shown as the source with a snippet of the error.

    $ cabal build all --dry-run
    ...
    Error: [Cabal-7090]
    Error parsing project file dir-elif/elif.config:4:
      - dir-elif/elif.config
          imported by: elif.project
      - Failed to parse 'elif(_)' with error:
        "<condition>" (line 1, column 1): unexpected SecArgName (Position 1 6) "_"
    

@philderbeast philderbeast force-pushed the fix/import-parse-error-location branch 3 times, most recently from 7c990c9 to a74ddc3 Compare December 16, 2024 19:09
@philderbeast
Copy link
Collaborator Author

@gbaz what are your concerns with the following comment and is there a test for this?

-- we rewrap as as a section so the readFields lexer of the conditional parser doesn't get confused
adaptParseError l (parseConditionConfVarFromClause . BS.pack $ "if(" <> p <> ")") <*>

Comment on lines 310 to 331
<$> adaptParseError l (parseConditionConfVarFromClause . BS.pack $ "else(" <> p <> ")")
<$> ( let s = "elif(" <> p <> ")"
in projectParse (Just s) source (adaptParseError l (parseConditionConfVarFromClause $ BS.pack s))
)
Copy link
Collaborator Author

@philderbeast philderbeast Dec 16, 2024

Choose a reason for hiding this comment

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

@gbaz was this a typo, using "else(" instead of "elif("?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Mikolaj, @ffaf1 and @geekosaur, I didn't hear back about this. Here's the code on the master branch:

parseElseClauses x = case x of
(ParseUtils.Section _l "else" _p xs' : xs) -> do
subpcs <- go [] xs'
rest <- go [] xs
pure (Just <$> subpcs, rest)
(ParseUtils.Section l "elif" p xs' : xs) -> do
subpcs <- go [] xs'
(elseClauses, rest) <- parseElseClauses xs
let condNode =
(\c pcs e -> CondNode mempty mempty [CondBranch c pcs e])
<$> adaptParseError l (parseConditionConfVarFromClause . BS.pack $ "else(" <> p <> ")")
<*> subpcs
<*> elseClauses
pure (Just <$> condNode, rest)
_ -> (\r -> (pure Nothing, r)) <$> go [] x

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's ping @gbaz again, maybe he missed the previous notification.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ffaf1 Gershom hasn't been answering my pings from this repository lately, so I'd not hold my breath. Of course, I'm happy to be wrong here!

@philderbeast philderbeast force-pushed the fix/import-parse-error-location branch 2 times, most recently from deec1f5 to 6f5239b Compare December 16, 2024 19:33
@philderbeast philderbeast force-pushed the fix/import-parse-error-location branch from 6f5239b to 9f05ce2 Compare December 16, 2024 20:05
@philderbeast philderbeast marked this pull request as draft December 17, 2024 15:33
@philderbeast
Copy link
Collaborator Author

philderbeast commented Dec 17, 2024

Reverting to draft while I settle some Windows versus POSIX file path issues.

@philderbeast philderbeast force-pushed the fix/import-parse-error-location branch 3 times, most recently from d45311c to 829c24d Compare December 22, 2024 20:15
@philderbeast philderbeast force-pushed the fix/import-parse-error-location branch 2 times, most recently from d42c82a to 9e745ee Compare December 30, 2024 17:52
@philderbeast philderbeast force-pushed the fix/import-parse-error-location branch from 9e745ee to d80b5ec Compare January 6, 2025 21:39
@ulysses4ever
Copy link
Collaborator

Instead of keep working on this, @philderbeast, do you want to help reviewing this: #8889?

@philderbeast philderbeast marked this pull request as ready for review January 7, 2025 01:09
@philderbeast
Copy link
Collaborator Author

Instead of keep working on this, @philderbeast, do you want to help reviewing this: #8889?

This has been ready for 3 weeks but I put it in draft while I waited for #10646, needed for asserting on the multiline output.

@philderbeast philderbeast force-pushed the fix/import-parse-error-location branch from f2b83d7 to c16f967 Compare January 9, 2025 22:36
@philderbeast philderbeast requested a review from jgotoh January 10, 2025 22:46
@philderbeast philderbeast force-pushed the fix/import-parse-error-location branch 3 times, most recently from 5086730 to 1bc9cff Compare January 16, 2025 18:48
Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

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

Good as far as I can see!

@philderbeast philderbeast force-pushed the fix/import-parse-error-location branch from 1bc9cff to 42f3609 Compare January 17, 2025 11:48
- Track which file has errors and which has warnings
- Add test for import parse warnings
- Remove added type sigs, use e for error type
- Move ProjectParseResult into its own module
- Import qualified from Deprecated.ParseUtils
- Reverse warnings so they are in line number order
- Report parse result error in imported config
- Split project test into warning and error tests
- Add type synonyms for project parse
- Extract function reportProjectParseWarnings
- Show the snippet that doesn't parse
- Add if, elif and else test projects
- Fix else for elif typo
- Show provenance if not root
- Rerun expected output with provenance
- Redo ParseWarningProvenence with ordered output
- Add ProjectParseError record
- Reword badly formed comment lines
- Satisfy fix-whitespace
- Add changelog entry
- Updated - indented expectation
- No snippet when modifying compiler under condition
- Only show custom message with snippet
- Rerun expected output with source
- Use a Doc for the ReportParseResult message
- Update expected .out files
- Use normalized path when recursing
- Consistent projectParse ... source
- Consistent projectParse ... normSource
- Use normalizeWindowsOutput
- Use .md extension on changelog entry
- Satisfy HLint
@philderbeast philderbeast force-pushed the fix/import-parse-error-location branch from 42f3609 to 38ce204 Compare January 17, 2025 11:51
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.

Warning source repeatedly wrong with unrecognized section
4 participants