KE2: basic external class extraction #18134
Conversation
| fileExtractionProblems.setNonRecoverableProblem() | ||
| */ | ||
| loggerBase.error(dtw, "Extraction failed while extracting '${psiFile.virtualFilePath}'.", e.stackTraceToString()) |
There was a problem hiding this comment.
We have a Logger.error(msg: String, exn: Throwable); it's probably more consistent to add one to LoggerBase too (and BasicLogger?) than to do the stackTraceToString here.
| val name = | ||
| when (element) { | ||
| is KtFile -> element.virtualFilePath | ||
| is KtNamed -> element.getNameAsName()?.asString() ?: "<missing name>" | ||
| else -> "<no name>" | ||
| } | ||
| val loc = tw.getLocationString(element) | ||
| val loc = element?.let { tw.getLocationString(it) } ?: "<unknown location>" |
There was a problem hiding this comment.
I assume this is for the null declaration.psiSafe() case below. I think it would be better to generalise the element parameter, or to add a withSymbol alternative function, so that we only have non-null elements. After all, when this appears in a stack trace, it's more useful to know that we were extracting the symbol for the declaration, rather than believe we were extracting the source declaration itself (or worse, have no info on what we were extracting).
| @@ -433,6 +430,10 @@ open class FileTrapWriter( | |||
| return getLocation(file, range) | |||
| } | |||
|
|
|||
| fun getFileOnlyLocation(): Label<DbLocation> { | |||
| return getLocation(fileId, 0, 0, 0, 0) | |||
There was a problem hiding this comment.
This already exists as getWholeFileLocation
| logger.warnElement("Unrecognised class kind $kind", e) | ||
| else | ||
| logger.warn("Unrecognised class kind $kind") | ||
| } |
There was a problem hiding this comment.
A few thoughts on this:
No matter what, I think we should wrap whatever pattern we end up using in a logger.warnSymbol method.
Even when there isn't a psiSafe, I think we can still get at least a name from the symbol.
I'm not sure if using the location of psiSafe is going to be useful or not; we're not actually extracting the source at this point, so is giving the source location going to send us in the wrong direction?
There was a problem hiding this comment.
I've gone for preserving psiSafe's location info if we can, since it is strictly more informative than symbol location. We can remove this if it proves not to be useful. I also note when inline generics are back in play we'll need to retrieve symbol locations differently since they won't always be extracted to the FileTrapWriter's fileId.
| @@ -56,7 +62,7 @@ fun KotlinFileExtractor.extractClassSource( | |||
| } | |||
| } | |||
|
|
|||
| val locId = tw.getLocation(c.psiSafe() ?: TODO()) | |||
| val locId = c.psiSafe<KtElement>()?.let { tw.getLocation(it) } ?: tw.getFileOnlyLocation() | |||
There was a problem hiding this comment.
I don't think we want to do this. If we can see the source, then we'll get the location when we extract the source. If we're here, we should just give the whole-file location.
There was a problem hiding this comment.
I think this is that place, though? In KE1 at least this function was shared between the extract-kt-source-class and extract-class-file paths, and it is shared here too since it works at symbol level. I'm not aware of another place where the whole class would get a source-file location ascribed if we know of one.
| return id | ||
| } | ||
|
|
||
| private fun tryReplaceType( |
There was a problem hiding this comment.
Could you add a comment giving the purpose and behaviour of this method please?
| */ | ||
| extractClassLaterIfExternal(replacedClass) | ||
| // TODO: This shouldn't be done here, but keeping it simple for now |
smowton
force-pushed
the
smowton/ke2/external-class-extraction
branch
from
d2323e9
to
d27b5ed
Compare
|
@igfoo comments addressed. |
Mainly just makes
Ktclasses optional to the point that basic external type extraction works, adds basic type parameter type extraction so we can at least tolerate seeing generics, and switches the external declaration extractor back on.