rename variables by default to avoid execute() clashes in linked libs

[mjrodgers]

This should fix oscar-system/Oscar.jl#1905 with variable names clashing with names used in Singular libraries.

We now append an #<number> to all variables by default (we also remove a potential infinite loop in the previous version of the hack).

[mjrodgers]

I'm open to using some other naming strategy, there was some talk about maybe prepending everything with something like @OSCAR@ or the like. My current fix was just the easiest thing that used (mostly) the method that was already there.

[mjrodgers]

Okay, the current rename function now does the following:

if the variable name x is "bad" (at the moment this means it contains characters that aren't allowed, or x == "minpoly") a transformation is applied to replace bad characters in a meaningful way, if this still results in a "bad" variable name then x is replaced with some default string (this is all left in place from the previous hack)
since there is no way to catch all of the variable names that will give problems, we replace the string x with "@oscar@"x"@"*string(i) for an integer i that makes sure all of the variable names are unique.
This should completely avoid clashes with any variable names used in the Singular libraries. We only send ASCII variable names. In fact, we probably don't need to filter out x == "minpoly" anymore, but I supposed it doesn't hurt to leave it.

[mjrodgers]

Oh dear, this causes basically all doctests to fail from renamed variables. I'm guessing this means that the previous version of rename_symbol was never being called by anything from the doctests, since it would give this same issue (except only in the few cases where there was a "bad" variable name)

[fingolfin]

I don't think we should update the variable names here. Instead I would perform this kind of renaming in Oscar.jl

[thofma]

Here in the docstring? Or altogether?

[mjrodgers]

I at least agree that if I am renaming the variables in Singular.jl, then prepending @OSCAR@ is not the right approach. I went with this because Singular.jl already had a helper function to rename "bad" variable names. I can create one in Oscar.jl, but when to call it becomes an issue. Because this issue is not just with computing primary decomposition from Singular, we could potentially run into these name clashes any time we call the Singular library.

Unrelated, at the moment this renaming is causing a segfault in the tests, during one of the calls to libSingular.algExt_GetMinpoly, trying to figure out what is going on with that.

[mjrodgers]

Looking into this more, the renaming is sort of a fundamental part of Singular.jl. The type constructors convert to a "good" list of variable names and write them to the Singular objects that they wrap, while maintaining a vector of the corresponding symbols used for the variable names in Julia. I think it would probably break some things to remove this from Singular.jl, and would require some type piracy to handle all of this from the Oscar.jl side.

I think Hans has helped identify the source of the segfault, but regardless, I would honestly vote for just considering every variable name bad, and replace it by def*<number> where def is the (already implemented) default taken by rename_symbol. The reasons being,

1. it's infeasible to predict all of the possible identifiers that will create a clash in the Singular library and
2. from the Julia side, we shouldn't care what Singular is calling these variables internally.

There are only 3 times when we might see these Singular variable names, and it will always be I think an issue that should be fixed:

1. We have Singular actually printing the output for some reason.
2. We have a badly defined print routine for an object that is pulling the variable names from the wrapped Singular object instead of from the list of julia symbols the object is using.
3. We for some reason are creating a julia object directly from a pointer to a Singular object, in this case it reads the variable names from Singular.

Related to 2. above, the Singular.jl polynomial rings have a bad print routine that shows the wrapped variable names (and is in general unintelligeable):

julia> using Singular;
julia> F, (a,) = FunctionField(QQ,["a"]);
julia> K, a = AlgebraicExtensionField(F, a^2 + 1);
julia> R, (x, y, z) = polynomial_ring(K, ["x", "y", "z"])
(Singular Polynomial Ring (0,a_1),(x_1,y_1,z_1),(dp(3),C), spoly{n_algExt}[x, y, z])

julia> R.S
3-element Vector{Symbol}:
 :x
 :y
 :z

This is in my version that renames all variables, though this occured previously whenever a variable was renamed; we see that the original symbols are properly saved, but the print routine gives us... I'm not even sure what. (But this occurs in Singular.jl, Oscar.jl does not have this problem).

[fingolfin]

Why was this removed?

[mjrodgers]

Hmm, as near as I can tell, we check that each l[i] < l[i-1], then also that l[i] < l[j] for some random j in 1:i-1. It seems this second test is redundant, or am I missing something? For example when i=2 this just checks that l[2] < l[1] twice, correct? I can add this one back, no problem.

[mjrodgers]

(as for why I messed with it in this pull request, I needed to fix the other ordering tests because of the variable names, and was looking at this one to compare.)

[thofma]

What is the status of this? Do you still have concerns/objections @fingolfin?

[mjrodgers]

I'm out of town at a conference, but will be back this weekend - I've been having issues with weird test failures. I've fixed a lot that were based on comparing strings, I've tried to make them a bit more generic. But one that's failing now is the caller.LibNormal test in caller-test.jl. Here some finite fields are generated, and they are not recognized as being compatible - I can't pinpoint how the renaming might have caused this. But I can get back to troubleshooting it next week.

[mjrodgers]

Okay, I am working through this (hopefully last?) failing test, and I think I need some advice about what the purpose of the test is. This is here.

   F, a = FiniteField(5, 3, "b")
   R, (x, y, z) = polynomial_ring(F, ["x", "y", "z"])
   I = Ideal(R, a*z-x^4, z-y^6)
   l = Singular.LibNormal.normal(I)
   S = l[1][1][1]
   @test S isa Singular.PolyRing{n_GF}
   @test base_ring(gens(l[1][1][2][:norid])[1]*gens(S)[1]*a) == F

So the call to Singular.LibNormal.normal(I) gives us

1. the polynomial ring (let's call it S) l[1][1][1]
2. an ideal (call this one Id1) l[1][1][2][:norid] over S
3. an ideal (call this one Id2) l[1][1][2][:normap] over S

The problem is that, when the variable for the finite field gets renamed, then base_ring(S) is not recognized as being the same as F (even though these Julia objects wrap pointers for the same Singular object), and they are also not compatible, so this test fails.

So what is this test checking? If it is just checking that S is a polynomial ring over the proper field, maybe we would just want to replace that second test with a check that base_ring(S) has the same characteristic + degree as F. As long as that is true, it would be no problem to do this multiplication if needed by just changing the base ring of the polynomials.

Now, it would be nice if base_ring(S) was actually identified with F, I think this could in theory be accomplished by recognizing (while constructing S) that the Singular pointer to the base ring is already in use by Singular.jl and associated to F, and then S could be constructed properly with F as the base field. I think this would require maintaining a Dict/cache or pointers in use by Singular.jl so that whenever we try to create a ring from a Singular pointer, we consult this cache first. But I don't know how important this is, it might make more sense for me to just figure out how to get this test to pass again.

[mjrodgers]

This was fixed on the Oscar side in oscar-system/Oscar.jl#2662