facebookincubator
Generated code needs a prefix to prevent clash
After this reply https://github.com/facebookincubator/CG-SQL/pull/150#issuecomment-1302502196 I started thinking about it and discovered that the code generated doesn't have any prefix and this can lead to name clash with internal function/variable names like:
create proc cql_finalize_stmt(obj integer not null)
begin
end;
Generates this Lua code:
--[[
CREATE PROC cql_finalize_stmt (obj INTEGER NOT NULL)
BEGIN
END;
--]]
function cql_finalize_stmt(obj)
cql_contract_argument_notnull(obj, 1)
end
Generates this C code:
/*
CREATE PROC cql_finalize_stmt (obj INTEGER NOT NULL)
BEGIN
END;
*/
#define _PROC_ "cql_finalize_stmt"
#line 23
void cql_finalize_stmt(cql_int32 obj) {
#line 25 "lopp-pp.cql"
#line 25
}
#undef _PROC_
#line 27 "lopp-pp.cql"
For example if there was a default prefix (user definable or not) like ucql_ then the above example would became:
Generates this Lua code:
--[[
CREATE PROC cql_finalize_stmt (obj INTEGER NOT NULL)
BEGIN
END;
--]]
function ucql_cql_finalize_stmt(obj)
cql_contract_argument_notnull(obj, 1)
end
Generates this C code:
/*
CREATE PROC cql_finalize_stmt (obj INTEGER NOT NULL)
BEGIN
END;
*/
#define _PROC_ "cql_finalize_stmt"
#line 23
void ucql_cql_finalize_stmt(cql_int32 obj) {
#line 25 "lopp-pp.cql"
#line 25
}
#undef _PROC_
#line 27 "lopp-pp.cql"
Looking through the code I can see that there is rtdata.symbol_prefix and if I set it to rt_c.symbol_prefix="ucql_" I get the desired result for the C runtime (at least for function names) but doing the same in rt_lua does nothing (missing code).
But I don't see a way to set symbol_prefix through compiler flags or cql flags.
I can see that it's used in rt_objc with this macro RT_SYM_PREFIX and RT_IMPL_SYMBOL_PREFIX.
I'm not really sure if the cure isn't worse than the disease. The symbol prefix makes a big mess of things generally... it is possible to get a conflict but it's not easy and usually it's obvious what's happened.
Having a prefix feature in the C output -- and camel casing -- is one of my biggest regrets in the C codegen.
I'd rather just make the names more obscure or disallow user names that begin with cql_ or something.
The problem is that with more language backends (more can come like go, c#, rust, pascal, ...) the chances of getting the same CG-SQL code translated to any of then without name clashing is reduced without a prefix (customizable or not) because it's hard to ask for a CQ-SQL writer to know all backend languages keywords or standard library names.
The reverse is also a problem. If you add prefixes then when calling into CQL from say lua it looks weird and you have to know the magic prefixes. If you're trying to find the procedure you have to search for something different. I've been living in that world for years and it's horrible. The prefix is not nearly as bad as the option to convert snake_case to PascalCase which I think was a true disaster...
Still adding the rt support to LUA so that people have the option by configuring rt_common.c or adding an override in rt.c (which you're supposed to do with RT_EXTRA) is reasonable. It's a lot of work though...
Id did an experiment trying to add a prefix to the Lua code generator but as you've said it got a bit messy:
------------------------------- sources/cg_lua.c -------------------------------
index 1f2ec47d7,1f2ec47d7..a9749f816
@@@ -45,6 -45,6 +45,8 @@@ cql_noexport void cg_lua_cleanup() {
#define LUA_EXPR_PRI_UNARY 10
#define LUA_EXPR_PRI_HIGHEST 999
++#define IF_VAR_PREFIX(sem_type) ((sem_type & SEM_TYPE_VARIABLE) ? rt->symbol_prefix : "")
++
static void cg_lua_expr(ast_node *node, charbuf *value, int32_t pri);
static void cg_lua_stmt_list(ast_node *node);
static void cg_lua_get_column(sem_t sem_type, CSTR cursor, int32_t index, CSTR var, charbuf *output);
@@@ -308,7 -308,7 +310,7 @@@ static void cg_lua_emit_to_bool(charbu
static void cg_lua_to_bool(sem_t sem_type, charbuf *value) {
if (!is_bool(sem_type)) {
CHARBUF_OPEN(temp);
-- bprintf(&temp, "%s", value->ptr);
++ bprintf(&temp, "%s%s", rt->symbol_prefix, value->ptr);
bclear(value);
cg_lua_emit_to_bool(value, temp.ptr);
CHARBUF_CLOSE(temp);
@@@ -402,7 -402,7 +404,7 @@@ static void cg_lua_var_decl(charbuf *ou
Contract(!is_null_type(sem_type));
Contract(cg_main_output);
-- bprintf(output, "local %s", name);
++ bprintf(output, "local %s%s", rt->symbol_prefix, name);
cg_lua_emit_local_init(output, sem_type);
}
@@@ -520,14 -520,14 +522,14 @@@ static void cg_lua_scratch_var(ast_nod
// Set nullable output type to null.
static void cg_lua_set_null(charbuf *output, CSTR name, sem_t sem_type) {
-- bprintf(output, "%s = nil\n", name);
++ bprintf(output, "%s%s = nil\n", rt->symbol_prefix, name);
}
// Once we've done any type conversions for the basic types we can do pretty simple assignments
// The nullable non-reference types typically need of the helper macros unless it's an exact-type copy
// operation. This function is used by cg_lua_store near the finish line.
static void cg_lua_copy(charbuf *output, CSTR var, sem_t sem_type_var, CSTR value) {
-- bprintf(output, "%s = %s\n", var, value);
++ bprintf(output, "%s%s = %s\n", rt->symbol_prefix, var, value);
}
// This is most general store function. Given the type of the destination and the type of the source
@@@ -641,14 -641,14 +643,14 @@@ static void cg_lua_binary(ast_node *ast
bprintf(value, "nil");
}
else if (force_call || is_nullable(sem_type_left) || is_nullable(sem_type_right)) {
-- bprintf(value, "cql_%s(%s, %s)", op_name, l_value.ptr, r_value.ptr);
++ bprintf(value, "cql_%s(%s%s, %s%s)", op_name, IF_VAR_PREFIX(sem_type_left), l_value.ptr, IF_VAR_PREFIX(sem_type_right), r_value.ptr);
}
else {
if (lua_needs_paren(ast, pri_new, pri)) {
-- bprintf(value, "(%s %s %s)", l_value.ptr, op, r_value.ptr);
++ bprintf(value, "(%s%s %s %s%s)", IF_VAR_PREFIX(sem_type_left), l_value.ptr, op, IF_VAR_PREFIX(sem_type_right), r_value.ptr);
}
else {
-- bprintf(value, "%s %s %s", l_value.ptr, op, r_value.ptr);
++ bprintf(value, "%s%s %s %s%s", IF_VAR_PREFIX(sem_type_left), l_value.ptr, op, IF_VAR_PREFIX(sem_type_right) , r_value.ptr);
}
}
@@@ -2145,7 -2145,7 +2147,7 @@@ static void cg_lua_emit_contracts(ast_n
bool_t notnull = is_not_nullable(sem_type);
if (notnull) {
-- bprintf(b, " cql_contract_argument_notnull(%s, %d)\n", name, position);
++ bprintf(b, " cql_contract_argument_notnull(%s%s, %d)\n", rt->symbol_prefix, name, position);
did_emit_contract = true;
}
}
@@@ -2177,7 -2177,7 +2179,7 @@@ static void cg_lua_emit_fetch_results_p
cg_lua_params(params, &args, &returns);
}
-- bprintf(decl, "function %s(%s)\n", fetch_results_sym.ptr, args.ptr);
++ bprintf(decl, "function %s(%s%s)\n", fetch_results_sym.ptr, rt->symbol_prefix, args.ptr);
CHARBUF_CLOSE(returns);
CHARBUF_CLOSE(args);
@@@ -2231,11 -2231,11 +2233,11 @@@ static void cg_lua_emit_proc_prototype(
bprintf(proc_decl, "%s(_db_", proc_sym.ptr);
if (args.used > 1) {
bprintf(proc_decl, ", ");
-- bprintf(proc_decl, "%s", args.ptr);
++ bprintf(proc_decl, "%s%s", rt->symbol_prefix, args.ptr);
}
}
else {
-- bprintf(proc_decl, "%s(%s", proc_sym.ptr, args.ptr);
++ bprintf(proc_decl, "%s(%s%s", proc_sym.ptr, rt->symbol_prefix, args.ptr);
}
CHARBUF_CLOSE(returns);
@@@ -4310,7 -4310,7 +4312,7 @@@ static void cg_lua_user_func(ast_node *
// otherwise we can just copy the data since the variable is for sure
// an exact match for the call return by construction.
-- bprintf(cg_main_output, "%s = %s(%s)\n", returns.ptr, func_sym.ptr, args.ptr);
++ bprintf(cg_main_output, "%s = %s(%s%s)\n", returns.ptr, func_sym.ptr, rt->symbol_prefix, args.ptr);
if (proc_as_func && dml_proc) {
// cascade the failure
@@@ -4512,7 -4512,7 +4514,7 @@@ static void cg_lua_call_stmt_with_curso
if (returns.used > 1) {
bprintf(cg_main_output, "%s = ", returns.ptr);
}
-- bprintf(cg_main_output, "%s(%s)\n", proc_sym.ptr, args.ptr);
++ bprintf(cg_main_output, "%s(%s%s)\n", proc_sym.ptr, rt->symbol_prefix, args.ptr);
if (dml_proc) {
// if there is an error code, check it, and cascade the failure
@@@ -5021,7 -5021,7 +5023,7 @@@ static void cg_lua_proc_result_set(ast_
cg_lua_params(params, &args, &returns);
}
-- bprintf(d, " %s = %s(%s)\n", returns.ptr, proc_sym.ptr, args.ptr);
++ bprintf(d, " %s = %s(%s%s)\n", returns.ptr, proc_sym.ptr, rt->symbol_prefix, args.ptr);
bprintf(d, " ");
if (dml_proc) {
cg_lua_error_on_not_sqlite_ok();
@@@ -5072,7 -5072,7 +5074,7 @@@
cg_lua_params(params, &args, &returns);
}
-- bprintf(d, " %s = %s(%s)\n", returns.ptr, proc_sym.ptr, args.ptr);
++ bprintf(d, " %s = %s(%s%s)\n", returns.ptr, proc_sym.ptr, rt->symbol_prefix, args.ptr);
bprintf(d, " ");
cg_lua_error_on_not_sqlite_ok();
----------------------------- sources/rt_common.c -----------------------------
index 8e7d2a212,8e7d2a212..c23ca784d
@@@ -49,7 -49,7 +49,7 @@@ static rtdata rt_c =
.symbol_case = cg_symbol_case_snake,
.generate_type_getters = 0,
.generate_equality_macros = 1,
-- .symbol_prefix = "",
++ .symbol_prefix = "ucql_",
.symbol_visibility = "extern ",
.cql_contract = "cql_contract",
.cql_log_database_error = "cql_log_database_error",
@@@ -124,7 -124,7 +124,7 @@@ static rtdata rt_lua =
.symbol_case = cg_symbol_case_snake,
.generate_type_getters = 0,
.generate_equality_macros = 1,
-- .symbol_prefix = ""
++ .symbol_prefix = "ucql_"
};
static rtdata rt_objc = {
Hello my friend, I'm sorry but I was laid off today, so I won't be able to help land this. I don't know who will take over or how long before they come up for air and are able to look at OSS issues.
Thank you for your contributions, it was a pleasure working with you. I may make a personal fork of this to make rico-ql but I dunno.
I'm sorry and I would be glad to continue cooperating in any OSS with you.