Skip to content

feat(parser): improve rule detection, inline recipe parsing, and PHONY handling #184

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.

Sign up for GitHub

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

Jump to bottom
Merged
mikelolasagasti merged 1 commit into from
Nov 4, 2025
Merged

feat(parser): improve rule detection, inline recipe parsing, and PHONY handling #184

mikelolasagasti merged 1 commit into from
Nov 4, 2025

Conversation

@mikelolasagasti
Copy link
Collaborator

@mikelolasagasti mikelolasagasti commented Oct 28, 2025

This patch significantly improves the Makefile parser and related rules:

  • Parser:

    • Support targets with special characters, patterns, and variables (%.o, file.ext, $(DIR_VAR), ${DIR_VAR}/subdir)
    • Add handling for inline recipes (target: deps ; command)
    • Recognize multiple-target rules (target1 target2 : dep)
    • Treat .PHONY and .DEFAULT_GOAL as rules (not variables)
    • Extend variable parsing to support ?=, !=, and += operators
    • Add detailed debug logging for ambiguous lines

  • Rules:

    • Update phonydeclared and minphony to support both parser modes (old .PHONY as variable and new .PHONY as rule)
    • Remove legacy off-by-one adjustment on PHONY line numbers. Fixes inaccurate violation line reporting (+1 line shift)
    • Preserve fallback to -1 when PHONY is missing

  • Tests:

    • Add new parser coverage for:
      • Inline recipes
      • Variable and pattern targets
      • Special characters, multiple targets, and file extensions
      • Other variable assignments (?=, !=, +=)
    • Adjust expected violation line numbers (from 21 to 22) for accurate reporting
    • Confirm full backward compatibility with existing fixtures

Overall, this modernizes the Makefile parser for complex rule syntax and brings line number reporting in sync with actual source positions.

Checklist

Not all of these might apply to your change but the more you are able to check
the easier it will be to get your contribution merged.

  • CI passes
  • Description of proposed change
  • Documentation (README, docs/, man pages) is updated
  • Existing issue is referenced if there is one
  • Unit tests for the proposed change

@mikelolasagasti
Copy link
Collaborator Author

mikelolasagasti commented Oct 28, 2025

This covers most of the complains in #61 but parser still has limitations and a different approach might be needed.

@mikelolasagasti mikelolasagasti force-pushed the partial-fix-61 branch 2 times, most recently from 4d37ef4 to 8730380 Compare October 29, 2025 09:26
@mikelolasagasti
Copy link
Collaborator Author

mikelolasagasti commented Oct 29, 2025

With latest changes:

  • Added fix and test TestParse_VariableLikeRuleIsNotRule to avoid target detection for blocks like:
ifeq ($(BUILD_GOOS),windows)
        EXTENSION := .exe
endif
  • Renamed reFindSpecialVariable to reFindSpecialTarget

@mikelolasagasti
Copy link
Collaborator Author

mikelolasagasti commented Oct 29, 2025

With latest changes:

  • Exclude .PHONY from uniquetargets rule as having multiple entries is OK, although it's a best practice to have just one.

@mikelolasagasti mikelolasagasti force-pushed the partial-fix-61 branch 2 times, most recently from f9b35e7 to fcef697 Compare October 29, 2025 10:25
@mikelolasagasti
feat(parser): improve rule detection, inline recipe parsing, and PHON…
1cbeee0 
…Y handling

This patch significantly improves the Makefile parser and related rules:

- Parser:
  - Support targets with special characters, patterns, and variables
    (`%.o`, `file.ext`, `$(DIR_VAR)`, `${DIR_VAR}/subdir`)
  - Add handling for inline recipes (`target: deps ; command`)
  - Recognize multiple-target rules (`target1 target2 : dep`)
  - Treat `.PHONY` and `.DEFAULT_GOAL` as rules (not variables)
  - Extend variable parsing to support `?=`, `!=`, and `+=` operators
  - Add detailed debug logging for ambiguous lines

- Rules:
  - Update `phonydeclared` and `minphony` to support both parser modes
    (old `.PHONY` as variable and new `.PHONY` as rule)
  - Remove legacy off-by-one adjustment on PHONY line numbers. Fixes
    inaccurate violation line reporting (+1 line shift)
  - Preserve fallback to -1 when PHONY is missing

- Tests:
  - Add new parser coverage for:
    - Inline recipes
    - Variable and pattern targets
    - Special characters, multiple targets, and file extensions
    - Other variable assignments (`?=`, `!=`, `+=`)
  - Adjust expected violation line numbers (from 21 to 22) for accurate
    reporting
  - Confirm full backward compatibility with existing fixtures
  - Skip `.PHONY` for uniquetargets rule as multiple entries is valid

Overall, this modernizes the Makefile parser for complex rule syntax
and brings line number reporting in sync with actual source positions.

Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
@mikelolasagasti
Copy link
Collaborator Author

mikelolasagasti commented Oct 29, 2025

As this PR improves detection multi-line recipe bodies, it causes maxbodylength errors to https://github.com/reactive-firewall-org/multicast/blob/master/Makefile

Now:

$ ./checkmake /tmp/multicast/Makefile 
     RULE           DESCRIPTION             FILE NAME         LINE NUMBER 
 minphony       Required target      /tmp/multicast/Makefile  174         
                "all" is missing                                          
                from the Makefile.                                        
 maxbodylength  Target body for      /tmp/multicast/Makefile  178         
                "MANIFEST.in"                                             
                exceeds allowed                                           
                length of 8 lines                                         
                (27).                                                     
 maxbodylength  Target body          /tmp/multicast/Makefile  292         
                for "just-test"                                           
                exceeds allowed                                           
                length of 8 lines                                         
                (13).                                                     
 maxbodylength  Target body for      /tmp/multicast/Makefile  311         
                "test-mat-doctests"                                       
                exceeds allowed                                           
                length of 8 lines                                         
                (13).                                                     
 maxbodylength  Target body for      /tmp/multicast/Makefile  326         
                "test-mat-%"                                              
                exceeds allowed                                           
                length of 8 lines                                         
                (12).                                                     
 maxbodylength  Target body for      /tmp/multicast/Makefile  340         
                "test-extra"                                              
                exceeds allowed                                           
                length of 8 lines                                         
                (12).                                                     
 maxbodylength  Target body for      /tmp/multicast/Makefile  354         
                "test-extra-%"                                            
                exceeds allowed                                           
                length of 8 lines                                         
                (12).                                                     
 maxbodylength  Target body for      /tmp/multicast/Makefile  368         
                "test-fuzzing"                                            
                exceeds allowed                                           
                length of 8 lines                                         
                (12).                                                     
 maxbodylength  Target body          /tmp/multicast/Makefile  382         
                for "test-perf"                                           
                exceeds allowed                                           
                length of 8 lines                                         
                (12).                                                     
 maxbodylength  Target body for      /tmp/multicast/Makefile  492         
                "build-docs"                                              
                exceeds allowed                                           
                length of 8 lines                                         
                (11).                                                     
Error: violations found (10)

Before:

$ ./checkmake /tmp/multicast/Makefile 
   RULE       DESCRIPTION             FILE NAME         LINE NUMBER 
 minphony  Required target     /tmp/multicast/Makefile  173         
           "all" is missing                                         
           from the Makefile.                                       
Error: violations found (1)

@mikelolasagasti mikelolasagasti mentioned this pull request Oct 29, 2025
Closed
obnoxxx
obnoxxx reviewed Oct 29, 2025
View reviewed changes
Copy link
Collaborator

@obnoxxx obnoxxx 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 this PR, @mikelolasagasti !
This looks like a useful set of changes.

I would personally prefer to split this into multiple commits: one for each logical/conceptual change.

But feel free to merge it as it is if you prefer that.

This was linked to issues Oct 29, 2025
".PHONY<space>:" not judged as valid #63
Closed
False positive .PHONY violation on variables #61
Closed
Support VAR:=value #74
Closed
@mikelolasagasti mikelolasagasti mentioned this pull request Oct 29, 2025
Closed
@mikelolasagasti mikelolasagasti merged commit c9da2b6 into checkmake:main Nov 4, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@obnoxxx obnoxxx obnoxxx left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support VAR:=value ".PHONY<space>:" not judged as valid False positive .PHONY violation on variables

2 participants

@mikelolasagasti @obnoxxx