search

zeroc-ice / ice

All-in-one solution for creating networked applications with RPC, pub/sub, server deployment, and more.
https://zeroc.com
GNU General Public License v2.0
2.05k stars 592 forks source link

Report Unknown Doc-Comment Tags in Slice #3127

Closed InsertCreativityHere closed 1 week ago

InsertCreativityHere commented 2 weeks ago

This PR fixes #2088. Now, the Slice parser rejects unknown doc-comment tags (including @remarks), making it's situation impossible to hit.

It also adds a check which rejects multi-line @see tags, since these are disallowed and @see should always be of the form: @see IDENTIFIER and nothing else.

When the parser encounters either of these cases, we discard (ignore) the offending line, and issue a warning for it. I think that the predominant philosophy in compilers is that: "a badly formatted comment should never break your build". Since, on a logical level, compilers should be ignoring all comments in your source file.

Since we currently have no validation of comments, I also added a new "InvalidComment" diagnostic category, which this PR uses.

InsertCreativityHere commented 1 week ago

After thinking about it more. I think even a warning is too harsh. We shouldn't issue any diagnostics at all for unknown tags. Just silently ignore them, and move on.

There is no sensible reason to tolerate them.

It's important to remember that there's two purposes to doc-comments. One is that we use them to generate doc-comments in the mapped languages, yes. But another, equally important one, is to document the Slice definitions themselves. And Slice doc-comments support the full breadth of doxygen.

It just so happens that the Slice parser is only aware of a very limited subset of these. Because supporting and mapping tags takes time and there's just better things to spend that time on.

But we do not want to restrict users to only using the very small subset of Doxygen tags that we map in the generated code. This would rob them of the extensive functionality doxygen provides. And if there's any users out there who use this functionality (which we document as providing), the suggested change would completely break their builds.

TL;DR

Just because we the parser doesn't generate code for a doc-comment tag, does not make that tag invalid. There is no such thing as "Slice comment tags" there are only "Doxygen comment tags", and we should make no attempt to convert 'slice2xxx' into a Doxygen tag validator.

InsertCreativityHere commented 1 week ago

Just to be clear, I still that this approach where collect all the unknown tags and just slap them at the end of doc comments isn't correct. If the Slice parser isn't aware of a tag, it should just ignore it and move on.

Obviously, the tags that the parser is aware of, we should validate. But if a user wants to ensure that their doc comment is correct, slice2cpp isn't the correct tool for that. doxygen is.

InsertCreativityHere commented 1 week ago

After discussion, we've come to the following middle ground:

1) We agree that we shouldn't just slap the unknown tags into the code, we have to ignore and discard it. 2) We should issue warnings for unknown tags; silently ignoring them would be too surprising. 3) In the future, we should add parser support for other common Doxygen tags, to reduce the number of warnings users who are familiar with Doxygen will encounter.

InsertCreativityHere commented 1 week ago

I agree with renaming Comment to doc comment, and have opened an issue to do it: #3141. I'd rather delay all the renaming of comment -> doc comment until that PR, so it's all done together.