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

main: add nulltag/z, a new extra #4152

Merged
merged 12 commits into from
Dec 24, 2024
Merged

Conversation

masatake
Copy link
Member

@masatake masatake commented Dec 8, 2024

main: add nulltag/z, a new extra

Close #4151.

Consider a parser attempting to emit a null tag, a tag whose
name is the empty string '\0'.

Original Behavior:

    It warns "ignoring null tag..." if both parserDefinition::allowNullTag and
    tagEntryInfo::allowNullTag are unset.

    It does not warn if either parserDefinition::allowNullTag or
    tagEntryInfo::allowNullTag is set.

    It does not emit the null tag, even if allowNullTag is set.

With This Change: The code now emits the null tag if:

    Either parserDefinition::allowNullTag or tagEntryInfo::allowNullTag
    is set, and

    The --extras=+z (or --extras=+{nulltag}) option is specified.

TODO:
- versioning
- updating the hacking guide
- make readtags warn "unsupported" if it finds "!_TAG_FIELD_DESCRIPTION"

Copy link

codecov bot commented Dec 8, 2024

Codecov Report

Attention: Patch coverage is 88.04348% with 11 lines in your changes missing coverage. Please review.

Project coverage is 85.90%. Comparing base (2049cb3) to head (a82df36).
Report is 19 commits behind head on master.

Files with missing lines Patch % Lines
main/options.c 63.63% 8 Missing ⚠️
main/writer.c 90.32% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4152   +/-   ##
=======================================
  Coverage   85.90%   85.90%           
=======================================
  Files         239      239           
  Lines       58733    58797   +64     
=======================================
+ Hits        50453    50509   +56     
- Misses       8280     8288    +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@masatake masatake force-pushed the nulltag branch 4 times, most recently from 5b2636b to 0d23879 Compare December 8, 2024 15:09
@techee
Copy link
Contributor

techee commented Dec 8, 2024

Ah, OK, I just noticed this PR - probably a better approach than #4152.

We'll probably want to disable this warning in Geany too.

@masatake
Copy link
Member Author

masatake commented Dec 8, 2024

I will try merging #4153 to this pull request to demonstrate how to use tagEntryInfo::allowNullTag.

@masatake masatake added this to the 6.2 milestone Dec 11, 2024
@masatake masatake force-pushed the nulltag branch 2 times, most recently from 38097fe to 5b29668 Compare December 14, 2024 10:17
@masatake masatake changed the title main: add nulltag/0, a new extra main: add nulltag/z, a new extra Dec 14, 2024
@masatake masatake force-pushed the nulltag branch 2 times, most recently from 0f7c699 to 8ce3dcf Compare December 14, 2024 18:41
@masatake
Copy link
Member Author

After taking time to implement null tags, I found that ctags could not record null tags. When sorting, null tags come before pseudo tags.

$ sort -u -f < tags
	main/main.c	/^static void batchMakeTags (cookedArgs *args, void *user CTAGS_ATTR_UNUSED)$/;"	f	typeref:typename:void	file:
!_TAG_EXTRA_DESCRIPTION	anonymous	/Include tags for non-named objects like lambda/
...

These pull requests implement null tags on the u-tags output format. If we choose xref and/or json, maybe ctags can record null tags. However, I have to rewrite many codes in this pull request.

We have the man page for the format already.

Signed-off-by: Masatake YAMATO <[email protected]>
The original code assumed using "L:" as the last column.
This change fixes the wrong assumption.

Signed-off-by: Masatake YAMATO <[email protected]>
This is preparation for "null tag support".

Signed-off-by: Masatake YAMATO <[email protected]>
We have allowNullTag in parserDefinition already.  However, we may
lose the chance of signs of bugs if we turn
parserDefinition::allowNullTag on.

With tagEntryInfo:allowNullTag, we can write a null tag without losing
the chance.

Signed-off-by: Masatake YAMATO <[email protected]>
@techee
Copy link
Contributor

techee commented Dec 19, 2024

Just a question - are null tags actually needed? I haven't followed the discussion around them so I maybe missed something but at lest for Geany, the only missing piece would be to have some way to disable the warnings in the terminal when there is an attempt to generate such tags. I think null tags are rare enough and not useful for anything like navigation or completion so IMO not worth the necessary work to support them.

masatake and others added 5 commits December 19, 2024 22:09
Close universal-ctags#4151.

Consider a parser attempting to emit a null tag, a tag whose
name is the empty string '\0'.

Original Behavior:

    It warns "ignoring null tag..." if both parserDefinition::allowNullTag and
    tagEntryInfo::allowNullTag are unset.

    It does not warn if either parserDefinition::allowNullTag or
    tagEntryInfo::allowNullTag is set.

    It does not emit the null tag, even if allowNullTag is set.

With This Change: The code now emits the null tag if:

    Either parserDefinition::allowNullTag or tagEntryInfo::allowNullTag
    is set,

    The --extras=+z (or --extras=+{nulltag}) option is specified, and

    The output format supports to emit null tags.

The xref and json output formats support to emit null tags.

Signed-off-by: Masatake YAMATO <[email protected]>
In javascript it is legal to have e.g. {"": false}.  With the
original code the parser attempts to generate the tag for the empty
string which produces warning.

With setting allowNullTag member of tagEntryInfo, the parser can
generate such tags without producing warnings.

Actual tag emission is controlled by --extras=+{nulltag}.

This commit is based on the commit in universal-ctags#4153.

Co-Author: Masatake YAMATO <[email protected]>
In the original code, the names output of formats are defined in
main/options.c; processOutputFormat maps a name output of a format to
a writer.

This is a preparation for implementing --list-output-format option.
@masatake
Copy link
Member Author

We cannot support null tags in the u-ctags and e-ctags output formats. As I wrote, null tags can cause trouble in the sorting process.
I don't inspect the etags output format much.
I added code to support null tags in the xref and json output formats.

One quick way to suppress the null tag warning is to use "2> /dev/null." However, this approach is too rough.

Thinking about finer-grained control and splitting responsibilities in parsers, main, and output writers, it looks good to me to support null tags explicitly.

@masatake masatake marked this pull request as ready for review December 19, 2024 13:34
@techee
Copy link
Contributor

techee commented Dec 20, 2024

One quick way to suppress the null tag warning is to use "2> /dev/null." However, this approach is too rough.

I meant it for Geany so users don't see these warnings which they don't know what they are about.

@masatake
Copy link
Member Author

With this request, a parser can make a valid null tag by setting 1 to tagEntryInfo::allowNullTag. Based on #4153, I made the JavaScript parser utilize this new feature. As a result, the warning reported in #4134 is gone.

Though a parser makes a null tag, ctags emits it only when (1) a user sets nulltag extra and (2) the writer for output supports null tags. Geany doesn't set the nulltag extra and installs/uses a custom writer that does not support null tags. So, Geany never sees null tags, and its users never see them.

@techee
This pull request satisfies what you (and Geany) expect, doesn't it?
This pull request is more complicated than #4153 because this pull request also solves #4151.

@techee
Copy link
Contributor

techee commented Dec 20, 2024

The only objective I had was to get rid of the NULL tag warnings but the way I understand the code

https://github.com/universal-ctags/ctags/pull/4152/files#diff-ff182293515b49570ea35fee56766e751aacb80608b1caf39e1ccc3ede475c7aR1950

the warning will be emitted when NULL tags are disabled. So by default it will do the opposite of what I wanted to achieve.

But if I understand it correctly, if I change the writer that Geany uses so it declares that Geany supports NULL tags, the warning should go away and in Geany we could simply just filter out the generated NULL tags. Am I right?

@masatake
Copy link
Member Author

ctags warned when a parser makes a null tag.

With this pull request, ctags warns when a parser makes a null tag without setting 1 to allowNullTag in tagEntryInfo.
In other words, if the parser sets 1 to allowNullTag a tagEntryInfo, the warning for the tagEntryInfo goes away.

The default value of allowNullTag is 0. So, a parser must set 1 to allowNullTag explicitly to suppress the warning.

See makeJsNullTag in parsers/jscript.c in 3ef7832.

@techee
Copy link
Contributor

techee commented Dec 23, 2024

if the parser sets 1 to allowNullTag a tagEntryInfo, the warning for the tagEntryInfo goes away.

OK, in that case it's what I wanted. I thought that it's the writer from which the null tag support is taken but apparently not. Thanks.

@masatake masatake merged commit a484466 into universal-ctags:master Dec 24, 2024
81 checks passed
@masatake
Copy link
Member Author

If 1 is set to the allowNullTag member && the writer supports emitting null tags && the users specify --extras=+{nulltag}, nulltags are emitted.
If 1 is set to the allowNullTag member, ctags never warns for the tag.

I wrote these things to ctags(1) and the document that will be uploaded to docs.ctags.io.
If the description isn't clear enough, pull requests are welcome.

@masatake
Copy link
Member Author

TODO:

  • versioning
  • updating the hacking guide
  • make readtags warn "unsupported" if it finds "!_TAG_FIELD_DESCRIPTION"

About the versioning, I will release 6.2 soon after solving the nominated issues and merging the nominated pull requests.

I wrote about the allowNulTag member in the hacking guide.

We don't have to think about readtags because {e,u}-ctags writers don't support emitting null tags.

Now we are ready for #4157.

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

Successfully merging this pull request may close these issues.

2 participants