fix(make): keep plain target names that are also path prefixes

[15daksh-2003]

The make completion collapses /-separated target names as filesystem paths, so a target that is also the prefix of others (install with install/local, X with X/fast) is dropped and only its /-suffixed siblings are offered.

Fixes #1219

Approach:

• In _comp_cmd_make__truncate_non_unique_paths, record the candidates make actually reported in an is_target map (keyed on the original candidate). In the emit loop, additionally offer a candidate's plain form when it is both a reported target and a path node with children. Synthesized ancestor nodes (pure prefixes make never reported) are not in is_target, so the recursive-Makefile collapsing from Make target completion doesn't work with subdirectories #544/Handle make targets with / correctly #858 is unchanged.
• Coupled nospace fix: a plain target and its dir/ prefix can now coexist in an unspecified COMPREPLY order. Set compopt -o nospace when any candidate (not just the first) ends in /, so drilling into the prefix keeps working.
• Known limitation (pre-existing): a target whose name literally ends in / is still not surfaced.

[akinomyoga]

#1219 discusses a possibility of using the information of whether the name is a .PHONY target. I haven't carefully read the changes in this PR to understand it, and I'm not even sure if it is possible, but the present changes don't seem to consider it.

[15daksh-2003]

Few things on it @akinomyoga (if I am getting it right as to what you are referring to) :

I did weigh .PHONY. The PR keys on a different bit: the collapse stage previously didn't know which names make actually reported as targets (the stage separation you noted in #1219). It now carries that across — an is_target map marks the extracted names, and the collapse stage re-offers a candidate's plain form only when it's both a reported target and a path node with children. Pure synthesized prefixes (the recursive subdir/… case) are never in is_target, so #544/#858 collapsing is unchanged.

On .PHONY specifically: it is extractable (the -p dump marks blocks with # Phony target (prerequisite of .PHONY).), but as the criterion, I feel it's both insufficient and unnecessary here:

a. Insufficient — a plain install: rule not declared .PHONY is still a real, user-facing target that hits the same collapse; a phony-only rule keeps dropping it, i.e. only partially fixes #1219:

install: # not .PHONY -> phony-only STILL drops it
echo install
install/local:
install/strip:

With the approach in PR, both this and the CMake .PHONY case complete to install + install/.

b. Unnecessary — CMake marks its targets .PHONY, so is_target already covers the reported cases.

The two criteria only differ for a non-phony target that is also a path prefix and is genuinely directory-prep, e.g. sub: ; mkdir -p sub with sub/a sub/b: | sub. For the approach taken here, it offers sub alongside sub/; a phony-only rule would offer just sub/. I think offering sub is acceptable: make sub is valid, it matches pre-2.12 behavior, and it's statically indistinguishable from the legitimate non-phony install above — the only signal separating them is .PHONY, which misclassifies that case. It also needs no change to the portability-sensitive extraction awk.

[akinomyoga]

I don't know how seriously I should respond to the details of AI-generated text these days, but I guess short responses would suffice for LLM, which has an excellent ability of capturing the context.

a. Insufficient

Isn't that just the Makefile is broken?

b. Unnecessary

Yes, it's unnecessary to satisfy your purpose. However, it breaks another behavior, as indicated in the next paragraph in your response.

I think offering sub is acceptable:

This is the key point. First of all, there is a reason that subdirectory names are dropped, i.e., to be consistent with the normal pathname completions. That's independent of the present issue, where the non-subdirectory install is missing, or suffixed by /. Basically, I think that whether we switch to generating subdirectory names should be discussed independently. An unrelated change shouldn't be introduced to justify a possibly poor implementation, unless it is really necessary. I need to carefully review the present change and also consider other approaches to conclude its necessity.

[15daksh-2003]

The AI remark… genuinely not something you come across daily in communities

That said, I think I feel why it came across that way — and it's a genuine miss on my side, not going to dress it up.

I anchored on "make reported install as a target, so don't drop it" and never circled back to what that actually means: .PHONY. You're right that .PHONY is the thing that separates a logical install (no real directory — fine to surface) from a real sub/ that just happens to also be a target, and the is_target check can't tell those apart.

For what it's worth, the "statically indistinguishable" line was me reasoning past the obvious answer — I'd fixated on the CMake repro (all phony there) and generalized off the flat target list. And the non-phony install example was a weak justification anyway; it just gave me the nudge to go back and re-read the manual.

[15daksh-2003]

as for carrying over .PHONY info to collapsing stage..if relied on awk parser, it seems it would degrade to today's behavior on non-gnu make, which I don't know how much doing it would solve the core idea for issue

[15daksh-2003]

but yeah, the extracter is already only parses GNU's -p output it seems

[akinomyoga]

The AI remark… genuinely not something you come across daily in communities

It's unrelated to this PR, but I strongly feel that these days, being a maintainer of several projects. I've been thinking of what we got and what we lost in the OSS communications with the emergence of LLM. The LLM expands the human inputs into a large amount of text, which contains both something that makes sense and doesn't. When I directly chat with an LLM, I can simply ignore weird parts. The issue happens when it is used for real conversations. I can't tell which part of the weirdness arises from LLM and which part would be the OP's thoughts, so I am supposed to take a lot of time to respond to all of the points, including LLM quirks, which leaves me with an empty feeling.

I'm very comfortable with the style after you switch (though you might still use LLM to improve wording).

[akinomyoga]

I'm now trying to see if there is another solution. I haven't yet tested it, but my naive idea is like f09a37c (after simple refactoring 2c12190).

as for carrying over .PHONY info to collapsing stage..if relied on awk parser, it seems it would degrade to today's behavior on non-gnu make, which I don't know how much doing it would solve the core idea for issue

but yeah, the extracter is already only parses GNU's -p output it seems

Yeah, e.g., BSD make doesn't support it. We already assume GNU make for our completion.

[15daksh-2003]

I was just checking something...it seems upstream has introduced: make --print-targets( which I believe would fall on (4.4.90→4.5) ), which might mean moving enumeration off the -p dump. So reanchoring a new dependency on -p's phony prose...where we might move to print targets once it lands for enumeration....

[15daksh-2003]

at least the first stage would be better moving away from awk parsing helper way project has today...

[15daksh-2003]

not sure if .phony gating is that of a good idea anymore...only if we stick to the way of enumeration we are doing today for bash completion

[akinomyoga]

I was just checking something...it seems upstream has introduced: make --print-targets( which I believe would fall on (4.4.90→4.5) ), which might mean moving enumeration off the -p dump.

Thanks. Does that new option emit a machine-parseable format? That would be useful. Thank you for the information.

So reanchoring a new dependency on -p's phony prose...

The format of -p for phony targets seems stable at least for 24 years since 2002. I haven't checked older versions because I don't have the working binary of older versions immediately.

not sure if .phony gating is that of a good idea anymore...only if we stick to the way of enumeration we are doing today for bash completion

Even if the new make is shipped, we still need to support older versions of make at the same time. We may include the code to use --print-targets, but we still need a fallback of using the -p output.

[15daksh-2003]

Even if the new make is shipped, we still need to support older versions of make at the same time. We may include the code to use --print-targets, but we still need a fallback of using the -p output.

I agree to that. Beleiving maintainance also would only go up: with print targets on top of -p as fallback. Perhaps, i dont know but, something like --print-phony on upstream... idk if it has been discussed over there or if Paul had any opinions on it, but seems like wishful thinking at this point. I would check this there once. meanwhile, for now, gating on phony where we have -p dump seems the only way feasible at least

[15daksh-2003]

Does that new option emit a machine-parseable format?

"--print-targets will print all explicit, non-special targets defined in the makefiles, one per line, then exit with success. No recipes are invoked and no makefiles are re-built."

seems flat only, not structured like a json or something. seems would be trivially consumable by while read, grep, compgen -W

[akinomyoga]

Yeah, I cloned the repository from Savannah, built the master branch, and was observing the behavior. As you described, the new option seems to simply print only the target names [with no headers or prefixes (like phony: or file: in f09a37c), indicating the type of the targets]. Although it seems to drop some of the filename targets starting with ../, it basically prints all non-trivial targets, including both phony and non-phony ones. So there isn't a way to tell which ones are phony, unless the suggested --print-phony would be implemented, or unless relying on the -p output.

I also checked the -p output of the master branch, which seems consistent with the traditional behavior.

[15daksh-2003]

on your posted approach, a couple of things(from the commit):

{target#target:} — shouldn't it be ${target#phony:}

The is_phony_target = 0 in the starts_with($0, prefix) rule.....which fires on every line when the prefix is empty (make ), wiping the flag the marker just set.

[akinomyoga]

{target#target:} — shouldn't it be ${target#phony:}

Ah, yeah. Right. Thank you.

The is_phony_target = 0 in the starts_with($0, prefix) rule.....which fires on every line when the prefix is empty (make ), wiping the flag the marker just set.

Hmm, the existing implementation seems tricky. This means that an unrecognized line like # Phony target (prerequisite of .PHONY). has been triggering is_target_block = 1 in the current implementation, though the side effect is suppressed by target = ""; after is_target_block = 0; in /^# +File is an intermediate prerequisite/.

[15daksh-2003]

i tried testing above what u suggested. have modified the tests a bit, along with empty prefix thing and the other one

[akinomyoga]

The refactoring commit should be separated. Otherwise, it's hard to review the substantial changes. I've separated back the original commit of refactoring and force-pushed.

[15daksh-2003]

The refactoring commit should be separated. Otherwise, it's hard to review the substantial changes. I've separated back the original commit of refactoring and force-pushed.

I am not aware much of conventions for commits here. I carried forward from cmake's conventions for one commit per PR

[akinomyoga]

I am not aware much of conventions for commits here. I carried forward from cmake's conventions for one commit per PR

That doesn't apply to this project. The convention seems to largely vary depending on the project.

[15daksh-2003]

corrected for few over spills over 79 cols in test_make.py

[15daksh-2003]

@akinomyoga one question out of curiosity: i was seeing zsh way of doing things, and I believe they expose a mechanism with some flags for opt in "collapse" with different semantics offered, while I think the default remains to be something collapsing targeted when implemented. what are your thoughts on that?

[akinomyoga]

a mechanism with some flags for opt in "collapse" with different semantics offered,

That would touch the central policy of this project, which I asked about on the very first day when I joined the maintainer group. The answer has been

#539 (comment)

That is, we want to minimize the customizability unless necessary. There is a related argument in #720 (comment).

edit: I was forgetting, but I noticed that there has also been a related sentence in doc/api-and-naming.md.

[akinomyoga]

I added adjustments in the push 4cb0d6e..d047e2f. We now have confirmed the -p output with the latest master branch of make. is_target_block = 0; has been moved to be paired with is_phony_target = 0;.

I also adjusted the commit message to clarify that make 3.80 is merely the minimum version I checked; the format of # Phony target in the -p output might have existed even before. I added Co-authored-by to both commits to mention the mutual contributions.

[15daksh-2003]

Hmm, the existing implementation seems tricky. This means that an unrecognized line like # Phony target (prerequisite of .PHONY). has been triggering is_target_block = 1 in the current implementation, though the side effect is suppressed by target = ""; after is_target_block = 0; in /^# +File is an intermediate prerequisite/.

@akinomyoga couple of things (both sort of non blocking I believe in the scope of this):
a. I was going through the manual, and it seems that not all special targets are in the list we have in target header block. Like .NOTINTERMEDIATE, which got added in make 4.4 or .EXTRA_PREREQS.
b. the semantics around is_target_block, which get a bit twisted for empty prefix scenario...should we think of moving header rule above the gate? something like this:

/^[^#\t:%]+:/ {
  # A header line begins a new block; assume it is not a wanted target
  # until proven otherwise below.
  is_target_block = 0;
  is_phony_target = 0;
  target = "";
   ... special-target skips, unchanged ...
   ... hidden-target skip, unchanged ...
  # only process the targets the user wants
  if (!starts_with($0, prefix)) next;

  is_target_block = 1;
  target = $0;
   ...
   next;
 }

# skip every line outside a wanted target block
is_target_block == 0 { next; }

what are your thoughts on this? if we do decide to go with either or both, should this be a separate PR or this one only?

[akinomyoga]

a. I was going through the manual, and it seems that not all special targets are in the list we have in target header block. Like .NOTINTERMEDIATE, which got added in make 4.4 or .EXTRA_PREREQS.

I think the list in the present completion code hasn't been updated since it was added by b28d710 (2012). Those special targets aren't intentionally excluded. They can be added to the list. Or another option may be to exclude all all-caps targets starting with a period.

b. the semantics around is_target_block, which get a bit twisted for empty prefix scenario...should we think of moving header rule above the gate? something like this:

I also felt it while I was checking another PR #1577, but I haven't carefully thought about whether that is totally equivalent to the present handling. The code was originally contributed in the form of a sed script. The strange way of processing might be due to the limitations of the sed language. However, there might possibly be another reason for the present strange way.

If we can mathematically transform the present processing to the suggested one, then we can safely rewrite the processing in the suggested way. Otherwise, we want to think carefully about hypothetical inputs causing different results, and confirm that make -npq doesn't produce such a pattern. However, as long as it's working, we wouldn't bother with rewriting the code, which would be the reason that the present implementation hasn't been fixed.

You may modify the loop structure this time, with attention. Or you can simply add /^[^#\t:%]+:/ && before starts_with in the present implementation, which is safer but uglier.

[akinomyoga]

Since another PR #1577 for the make script was merged, I've rebased the PR head on top of the current main branch. Then, I modified the code added in #1577 to make it consistent with the change of this PR (2573ed2..b29d4ea).

[15daksh-2003]

a. I was going through the manual, and it seems that not all special targets are in the list we have in target header block. Like .NOTINTERMEDIATE, which got added in make 4.4 or .EXTRA_PREREQS.

I think the list in the present completion code hasn't been updated since it was added by b28d710 (2012). Those special targets aren't intentionally excluded. They can be added to the list. Or another option may be to exclude all all-caps targets starting with a period.

b. the semantics around is_target_block, which get a bit twisted for empty prefix scenario...should we think of moving header rule above the gate? something like this:

I also felt it while I was checking another PR #1577, but I haven't carefully thought about whether that is totally equivalent to the present handling. The code was originally contributed in the form of a sed script. The strange way of processing might be due to the limitations of the sed language. However, there might possibly be another reason for the present strange way.

If we can mathematically transform the present processing to the suggested one, then we can safely rewrite the processing in the suggested way. Otherwise, we want to think carefully about hypothetical inputs causing different results, and confirm that make -npq doesn't produce such a pattern. However, as long as it's working, we wouldn't bother with rewriting the code, which would be the reason that the present implementation hasn't been fixed.

You may modify the loop structure this time, with attention. Or you can simply add /^[^#\t:%]+:/ && before starts_with in the present implementation, which is safer but uglier.

will push something in a bit

[akinomyoga]

will push something in a bit

I think those should be done in separate PRs.

[15daksh-2003]

I think those should be done in separate PRs.

sure. fine with me. is there anything else left for this PR to close?

[akinomyoga]

Nothing from me. Let's wait to see if others have something.

[scop]

Nothing from me, quite frankly the make completion as a whole is already beyond something I'd like to touch or look at, but didn't catch anything alarming on a quick skim, and there are tests, and no existing tests needed modifications, so I'm fine. A quick LLM review didn't yield anything of substance that I can tell either.