Transpiler: Remove .unwrap() usages

[KieranKaelin]

The AST navigation in the transpiler is making substanial use of .unwrap() on Options. As these may panic, they should be refactored.

Refer to #70 (comment).

[justahero]

Some of the rules use unwrap to call the IdentGroup::name which returns an Option. For example in implementation ReplaceSysdate::find_rules.

poc-plpgsql-analyzer/src/rules/builtins.rs

Lines 57 to 68 in 75ed336

	fn find_rules(
	&self,
	root: &Root,
	_ctx: &DboAnalyzeContext,
	) -> Result<Vec<RuleMatch>, RuleError> {
	let locations: Vec<RuleMatch> = filter_map_descendant_nodes(root, IdentGroup::cast)
	.filter(|i| i.name().unwrap().to_lowercase() == "sysdate")
	.map(|i| RuleMatch::from_node(i.syntax()))
	.collect();
	Ok(locations)
	}

One possible option to avoid the unwrap call is to treat a missing name as a false case. The filter call could become

filter(|i| i.name().map_or(false, |i| i.to_lowercase() == "sysdate"))

which seems idiomatic enough. An alternative approach could be to expect IdentGroup::name to always return a String but this may lead to an inconsistent API. Otoh especially an IdentGroup should most probably always contain an identifier.

[justahero]

Another example is the ReplaceEpilogue::find_rules implementation:

poc-plpgsql-analyzer/src/rules/procedure.rs

Lines 121 to 127 in 75ed336

	let syntax_tokens = t
	.parent()
	.unwrap()
	.siblings_with_tokens(Direction::Next)
	.skip(1)
	.take(4)
	.collect::<Vec<_>>();

An alternative could look like:

let syntax_tokens = t
    .parent()
    .into_iter() // called exactly once (on Some) or zero (None)
    .flat_map(|it| it.siblings_with_tokens(Direction::Next)) // but requires to flatten inner Iterator then
    .skip(1)
    .take(4)
    .collect::<Vec<_>>();

This works quite nicely, because Option can be turned into an Iterator using Option::into_iter. If a call to t.parent() returns a None the iterator is not executed and results in an empty Vec, otherwise flat_map is called once. The call to flat_map is necessary, otherwise the return type would be a Vec of Iterators (or Vec of Vecs).

This iterator sequence can also be found in Rust Analyzer.

[justahero]

Found another type for eliminating unwrap calls. Often there is an unwrap in a closure where the ? operator cannot be applied. In some instances it's possible to adapt the current iterator in a way that only changes it slightly.

For example in the AddParamlistParenthesis::find_rules implementation.

poc-plpgsql-analyzer/src/rules/procedure.rs

Lines 29 to 41 in 75ed336

	let locations: Vec<RuleMatch> = filter_map_descendant_nodes(root, Procedure::cast)
	.filter_map(|p| p.header())
	.filter(|h| h.param_list().is_none())
	.map(|h| {
	RuleMatch::new(
	h.syntax().clone(),
	TextRange::at(
	h.identifier().unwrap().syntax().text_range().end(),
	0.into(),
	),
	)
	})
	.collect();

Note the map call, that returns a TextRange if valid. Changing the map into filter_map enables the use of the ? operator then. In case a None value is found, we can immediately leave the closure, similar to how a function would do.

The map call then becomes:

.filter_map(|h| {
    Some(RuleMatch::new(
        h.syntax().clone(),
        TextRange::at(
            h.identifier()?.syntax().text_range().end(),
            0.into(),
        ),
    ))
})

I think this covers a fair chunk of the cases that exist in the code. The unwrap calls in tests could stay in as this part of the expected behaviour. Some unwrap calls could also be changed into expect calls to also provide a reason or add some meaning.