Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

G-8310: Make guideline less strict by limiting the scope of data types #182

Closed
PhilippSalvisberg opened this issue Sep 30, 2022 · 1 comment
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@PhilippSalvisberg
Copy link
Collaborator

As mentioned in #181 the guideline is too strict IMO.

There are some cases where a violation can be considered a false positive. For example for the following data types (based on SQL Language Reference 19c) :

  • BINARY_FLOAT
  • BINARY_DOUBLE
  • LONG
  • LONG RAW
  • DATE
  • BLOB
  • CLOB
  • NCLOB
  • BFILE
  • ROWID
  • INTEGER
  • DOUBLE PRECISION
  • REAL
  • ...

For these datatypes it is not possible to define a size or a precision and scale. So the data type of the parameter is always fully qualified. It's not passible that a VALUE_ERROR is thrown when assigning the parameter value to a local variable of the same data type.

So, I suggest to limit the check to chosen data types, where we know that a size, precision or scale can be relevant and VALUE_ERROR is possible and in the responsibility of the caller. Here's the full list:

Data type Optional size/precision/scale/...? Comment
CHAR No
VARCHAR2 No
NCHAR Yes The default is a single character, the length of the parameter is undefined, hence relevant.
NVARCHAR2 No
NUMBER Yes Throws a value_error, hence relevant.
FLOAT Yes Value is rounded, does not throw a value_error, hence irrelevant.
RAW No
TIMESTAMP Yes Value is rounded, does not throw a value_error, hence irrelevant.
INTERVAL YEAR TO MONTH Yes Throws a value_error, hence relevant.
INTERVAL DAY TO SECOND Yes Throws a value_error, hence relevant.
UROWID Yes Does not throw a value_error, should not be used anyway, hence irrelevant
CHARACTER [VARYING] No
VARCHAR No
NATIONAL CHAR[ACTER] [VARYING] No
NUMERIC Yes Throws a value_error, hence relevant.
DECIMAL Yes Throws a value_error, hence relevant.
DEC Yes Throws a value_error, hence relevant.
...%TYPE Yes We do not know anything about the datatype, can be VARCHAR2 (relevant) or DATE (irrelevant), hence relevant.

When limiting the guideline to these data types some false negatives are possible. E.g when defining a constrained subtype. With static code analysis and a limited scope of a file that's probably unavoidable to reduce the number of false positives.

@PhilippSalvisberg PhilippSalvisberg self-assigned this Sep 30, 2022
@PhilippSalvisberg PhilippSalvisberg added the enhancement New feature or request label Sep 30, 2022
@PhilippSalvisberg
Copy link
Collaborator Author

PhilippSalvisberg commented Sep 1, 2023

g-7420 is an example where G-8310 is thrown for parameters of type pls_integer.

@PhilippSalvisberg PhilippSalvisberg added this to the v4.3 milestone Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant