SourceMap.lookup_line panics when position is not found

[paavohuhtala]

Describe the bug

I'm using SWC as a library in a dead code analysis tool. I noticed that swc_common::source_map::SourceMap.lookup_line panics when the given byte position cannot be found from the source map, for example when using SourceMap::default() as a placeholder. I assume this is unintentional, as the function returns a Result, and there is some error handling code in lookup_line_with. The issue seems to be in lookup_source_file, which explicitly panics when the source file is not found.

Input code

fn create_span_source(&self, span: Span) -> ModuleSourceAndLine {
    let line = self
        .source_map
        .0
        // This panics at the moment
        .lookup_line(span.lo())
        .map(|source_and_line| source_and_line.line)
        .unwrap_or(0);

    ModuleSourceAndLine::new(self.root_relative_path.clone(), line)
}

Config

No response

Playground link

No response

Expected behavior

I would expect lookup_source_file to do the necessary error handling so that lookup_line never throws under any regular circumstances.

Version

swc_common = "0.14.6"

Additional context

No response

[kdy1]

using SourceMap::default()

This is the problematic part.
All code of swc assumes that SourceMap has correct data.

[paavohuhtala]

That definitely makes sense as an implementation detail, but the API is still a bit misleading. The documentation for the function says:

/// If the relevant source_file is empty, we don't return a line number.

which I guess is technically true, but in my mind not returning a line number does not mean that the library panics. Perhaps we should just update the documentation to cover the case when the function panics?

[kdy1]

Yeah, I think it's good change.
I'll close this while adding docs about design of SourceMap

[sam-goodwin]

Ran into this problem when mapping a Span to line/col in a plugin. One of the nodes had a position of 0 which triggers this panic.

pub fn lookup_source_file_in(
        files: &[Lrc<SourceFile>],
        pos: BytePos,
    ) -> Option<Lrc<SourceFile>> {
        if pos.is_dummy() {
            return None;
        }

Would it be better to have the interface return Option instead of panic? For backwards compatibility purposes, a new try_lookup_char_pos could be added that is safer? Either way, this interface is problematic and scary.

[kwonoj]

Related: #5535 (Not the same issue, but related with error handling)

I agree hard panic is not something desirable as there's no way consumers to workaround / ignore failures.