Opened 3 years ago

Closed 3 years ago

#26958 closed defect (fixed)

Inheriting from MPolynomialRing_libsingular crashes sage

Reported by: etn40ff Owned by:
Priority: major Milestone: sage-8.7
Component: algebra Keywords: polynomial rings
Cc: Merged in:
Authors: Salvatore Stella Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 57c8182 (Commits, GitHub, GitLab) Commit: 57c81821cf0afc9060f2826002a56b9ff3563653
Dependencies: Stopgaps:

Status badges

Description

The following crashes sage with a SIGSEGV:

sage: from sage.rings.polynomial.multi_polynomial_libsingular import MPolynomialRing_libsingular
sage: class Foo(MPolynomialRing_libsingular):
....:     pass
sage: Foo(QQ, 2, ['x','y'], 'degrevlex')

Strangely enough this bug is triggered only when inheriting; indeed the following works as expected:

sage: from sage.rings.polynomial.multi_polynomial_libsingular import MPolynomialRing_libsingular
sage: MPolynomialRing_libsingular(QQ, 2, ['x','y'], 'degrevlex')
Multivariate Polynomial Ring in x, y over Rational Field

As it turns out this bug is due to a spurious call to __init_extra__ of Algebras(...).parent_class. Univariate polynomial rings avoid this call by setting the attribute _no_generic_basering_coercion = True; now multivariate polynomial rings do so too.

Change History (9)

comment:1 Changed 3 years ago by etn40ff

  • Branch set to u/etn40ff/26958
  • Commit set to d628f1459d21ecd34c14366336a7b56fc677b78c
  • Status changed from new to needs_review

New commits:

d628f14Fix 26958

comment:2 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review
  • Type changed from PLEASE CHANGE to defect

Doesn't hurt I suppose...

comment:3 Changed 3 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Although maybe a minor comment about the code: can you replace

cdef public bool _no_generic_basering_coercion

by

cdef readonly _no_generic_basering_coercion

and remove the bool cimport (which serves no purpose)?

comment:4 Changed 3 years ago by git

  • Commit changed from d628f1459d21ecd34c14366336a7b56fc677b78c to 57c81821cf0afc9060f2826002a56b9ff3563653

Branch pushed to git repo; I updated commit sha1. New commits:

57c8182Avoid importing bool

comment:5 Changed 3 years ago by etn40ff

Done, thanks for the review.

A side note worth noting for #25558: after this patch it is possible to inherit from MPolynomialRing_libsingular but it is not possible to change its element class because the element constructor uses a function, new_MP, which hardcodes its output.

comment:6 Changed 3 years ago by etn40ff

  • Status changed from needs_work to needs_review

comment:7 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:8 Changed 3 years ago by embray

  • Milestone changed from sage-8.6 to sage-8.7

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

comment:9 Changed 3 years ago by vbraun

  • Branch changed from u/etn40ff/26958 to 57c81821cf0afc9060f2826002a56b9ff3563653
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.