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.

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

feat: source location included in warnings for unresolved references #704

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

feat: source location included in warnings for unresolved references #704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions include/mrdocs/Metadata/Javadoc.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,13 +308,22 @@ struct Link : Text
struct Reference : Text
{
SymbolID id = SymbolID::invalid;
std::string fileName;
int line;
int column;

static constexpr Kind static_kind = Kind::reference;

explicit
Reference(
String string_ = String()) noexcept
String string_ = String(),
String fileName_ = String(),
uint32_t line_ = 0,
uint32_t column_ = 0) noexcept
: Text(std::move(string_), Kind::reference)
, fileName(std::move(fileName_))
, line(line_)
, column(column_)
{
}

Expand All @@ -328,8 +337,14 @@ struct Reference : Text
protected:
Reference(
String string_,
Kind kind_) noexcept
Kind kind_,
String fileName_,
uint32_t line_,
uint32_t column_) noexcept
: Text(std::move(string_), kind_)
, fileName(std::move(fileName_))
, line(line_)
, column(column_)
{
}
};
Expand All @@ -344,8 +359,11 @@ struct Copied : Reference

Copied(
String string_ = String(),
String fileName_ = String(),
uint32_t line_ = 0,
uint32_t column_ = 0,
Parts parts_ = Parts::all) noexcept
: Reference(std::move(string_), Kind::copied)
: Reference(std::move(string_), Kind::copied, std::move(fileName_), line_, column_)
, parts(parts_)
{
}
Expand Down
16 changes: 15 additions & 1 deletion src/lib/AST/ParseJavadoc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,9 +834,16 @@ visitInlineCommandComment(
close = std::ranges::count(ref, ')');
}
}
PresumedLoc const loc = sm_.getPresumedLoc(C->getBeginLoc());
auto filename = loc.getFilename();
auto line = loc.getLine();
auto column = loc.getColumn();
emplaceText<doc::Copied>(
C->hasTrailingNewline(),
ref,
filename,
line,
column,
convertCopydoc(ID));
return;
}
Expand All @@ -855,11 +862,18 @@ visitInlineCommandComment(
bool const hasExtraText = ref.size() != s.size();
if (!ref.empty())
{
PresumedLoc const loc = sm_.getPresumedLoc(C->getBeginLoc());
auto filename = loc.getFilename();
auto line = loc.getLine();
auto column = loc.getColumn();
// the referenced symbol will be resolved during
// the finalization step once all symbols are extracted
emplaceText<doc::Reference>(
C->hasTrailingNewline() && !hasExtraText,
std::string(ref));
std::string(ref),
std::string(filename),
line,
column);
}
// Emplace the rest of the string as doc::Text
if(hasExtraText)
Expand Down
8 changes: 2 additions & 6 deletions src/lib/Metadata/Finalize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,17 +190,13 @@ class Finalizer

if constexpr(std::derived_from<NodeTy, doc::Reference>)
{
#if 0
// This warning shouldn't be triggered if the symbol has
// been explicitly marked excluded in mrdocs.yml
if(! resolveReference(N))
{
report::warn("Failed to resolve reference to '{}' from '{}'",
N.string, current_->Name);
report::warn("Failed to resolve reference to '{}' from '{}' at file '{}' line {} column {}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

#528 describes one problem B: the need for source file information in warnings and errors. This is important.

However, the original problem A, which motivated the #if 0, is independent: this message is a false positive in the caller's context.

For instance, if I have these functions:

detail::my_impl func_a();

std::optional<int> func_b();

Because detail::my_impl is an implementation-defined type and std::optional is not part of my project, I would get messages like "Failed to resolve reference to 'detail::my_impl' from 'func_a()' at file 'X' line Y column Z" or "Failed to resolve reference to 'std::optional<int>' from 'func_b()' at file 'X' line Y column Z".

Having the line numbers improves the message (problem B). But that doesn't change the fact that the message is wrong (problem A) because these return types are not supposed to be documented, and the user is not obliged not to use undocumented types. So, the user is correct but still receiving a warning.

So, if we solve problem B without solving problem A, it doesn't justify removing #if 0 from the caller here. The possible solutions would be:

  1. Fixing these false negatives so the caller only prints messages when they are correct. The message would not be a false negative if, instead of the return type in code, the reference were an explicit @ref in the javadoc that can't be resolved.
  2. Leaving the #if 0 there, leaving the potential improved error messages there, and create another issue for these false negatives.

Also, regarding #528, he got this warning without the source location. However, the issue is about warnings and errors in general, and that's not the only warning MrDocs emits.

N.string, current_->Name, N.fileName, N.line, N.column);
}
#else
resolveReference(N);
#endif
}
});
}
Expand Down
Loading