Scaladoc: Refactor snippet compiler reporting #13351
Conversation
scaladoc/src/dotty/tools/scaladoc/snippets/SnippetCompiler.scala
Outdated
Show resolved
Hide resolved
pikinier20
force-pushed
the
scaladoc/snippet-compiler-reporting
branch
from
d3b17d0 to
d8e6e1b
Compare
| @@ -129,16 +129,12 @@ abstract class MarkupConversion[T](val repr: Repr)(using dctx: DocContext) { | |||
| val path = s.source.map(_.path) | |||
| val pathBasedArg = dctx.snippetCompilerArgs.get(path) | |||
| val data = SnippetCompilerDataCollector[qctx.type](qctx).getSnippetCompilerData(s, s) | |||
| val sourceFile: dotty.tools.dotc.util.SourceFile = s.pos.fold(dotty.tools.dotc.util.NoSource)(_.sourceFile.asInstanceOf[dotty.tools.dotc.util.SourceFile]) | |||
There was a problem hiding this comment.
Can we get rid of this cast as well? How should we handle failures?
There was a problem hiding this comment.
Yes, I'll refactor this cast. In 99.99% cases there will be source file associated with symbol, especially in end-users projects. Maybe we should just produce warning/info with information that snippet compiler couldn't obtain source files for certain snippets and due to that the positions might not be accurate?
There was a problem hiding this comment.
In which situation would there be no files?
There was a problem hiding this comment.
I'm not so fluent with the compiler internals but I can guess that there might be a problem with symbols created synthetically in compiler? For some reason the Position in Symbol is optional, maybe @smarter can provide there some information.
There was a problem hiding this comment.
I don't know the context here, if it's a symbol coming from sources it should always have a source associated with it, but you can use s.source to access that just like you do above instead of going through s.pos.
There was a problem hiding this comment.
s.source is available only using compiler API of symbol but we need to cast between Quotes API and Compiler API either way. I think that smarter's solution is better because it's used in some places of Scaladoc and seems stable. This way we get SourceFile instead of Option[SourceFile] but still we need to handle NoSource. Actually, I think that getting NoSource is not a failure and should be supported.
There was a problem hiding this comment.
Oh I didn't realize it wasn't available in the quotes API, yeah we should try to uniformize these things.
but we need to cast between Quotes API and Compiler API
This is something we absolutely need to get rid of in the future (scaladoc should only use the public API, anything missing in the public API needs to be added to it), so I would caution against making the problem worse by doing even more casting.
There was a problem hiding this comment.
99% of casting is done in file called SyntheticSupport, maybe we should take a look at it and create an issue containing all things missing in public API? That was actually a purpose of this file
There was a problem hiding this comment.
create an issue containing all things missing in public API?
sounds good yes
pikinier20
force-pushed
the
scaladoc/snippet-compiler-reporting
branch
from
d8e6e1b to
f8d4168
Compare
No description provided.