Expr constructor sometimes returns a Symbol, Bool, etc

[timholy]

Examples:

CSTParser.jl/src/conversion.jl

Lines 137 to 142 in 778212a

	Symbol("@", normalize_julia_identifier(valof(x.args[2])))
	else
	Symbol(normalize_julia_identifier(valof(x.args[2])))
	end
	else
	return Symbol(normalize_julia_identifier(valof(x)))

I worry it's troubling to claim to be a constructor of a type and not return that type. That method could be called expr_or_symbol, or it could always return an Expr the way that Base does.

EDIT: but _literal_expr is going to be real trouble. Maybe this should be expr_or_literal?

[pfitzseb]

Yeah, the meaning of this function is "what would Meta.parse return when called on the original string?". It's kinda convenient to have this be a Expr constructor, but I agree that we should use a more accurate name.

[timholy]

Another option would be to do a check in the caller:

if converts_to_expr(myex)
    ex = Expr(myex)
else
    # whatever else
end

I came here because of quite a large number of invalidations triggered by this function; making it more inferrable would eliminate those. Since your Expr method is recursive, for inferrability it probably always needs to return an Expr.

But I don't know enough about the package to make a quick tweak. You don't have to do anything about this (and I'm not sure I'm planning on it either), but I thought I'd report this just on the general principle that a constructor that lies is a bit confusing.

[KristofferC]

Is there a strong need for this to be a constructor for Expr or can it just be CSTParser.to_expr. Are there places where Expr is called and it is unknown if the input type is a CSTParser.EXPR or a normal Julia AST?

[pfitzseb]

I don't think this particular Expr constructor is used much at all except for tests, so yes, we can probably easily use a different name. I'll play around with this a bit.