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

Error -> Defect for defects #13908

Merged
merged 2 commits into from
Apr 28, 2020
Merged

Error -> Defect for defects #13908

merged 2 commits into from
Apr 28, 2020

Conversation

arnetheduck
Copy link
Contributor

The distinction between Error and Defect is subjective,
context-dependent and somewhat arbitrary, so when looking at an
exception, it's hard to guess what it is - this happens often when
looking at a raises list without opening the corresponding
definition and digging through layers of inheritance.

With the help of a little consistency in naming, it's at least possible
to start disentangling the two error types and the standard lib can set
a good example here.

@Araq
Copy link
Member

Araq commented Apr 7, 2020

Gah, the older names were more readable though. Maybe we can find some other solution?

@timotheecour
Copy link
Member

timotheecour commented Apr 7, 2020

The distinction between Error and Defect is subjective, context-dependent and somewhat arbitrary

my feeling as well. What if instead of these renamings we did the following (syntaxwise, identical to --hint flags)

--panics:on # Defects terminate (can't be caught)
--panics:list # will list all selected panics (eg from cmdline + cfg files)
--panic:OutOfMemError:on  --panic:AssertionError:on # these 2 Defects will become non-catchable

which is more flexible for users who care about maximizing performance but still want to allow one specific type of defect to be catchable, depending on their applicatin.

example

For eg, a numerical application may want to allow DivByZeroError as catchable (which keeps code clean and efficient avoiding to check for 0 everywhere, but still allows for possibility of divide by 0 due to numerical roundoffs), but make AssertionError a defect (again, for performance)

@Araq
Copy link
Member

Araq commented Apr 7, 2020

@timotheecour Switches like these mean that we effectively have different language dialects. IMHO we should simply not track Defects in the type system. Even if your proc doesn't do any arithmetic and no allocation whatsoever (not sure I've ever written such a proc btw!) the call itself could fail due to a StackOverflow.

@arnetheduck
Copy link
Contributor Author

Gah, the older names were more readable though.

who cares, you should never actually see them, right? :)

not sure I've ever written such a proc btw

you have, and lots of it - an api for add that returns (int, bool) doesn't have any defects or errors and matches the hardware it's running on - doesn't even even have stack exhaustion if you consider normal operating parameters where all that stuff fits in registers - and if you're building a bigint library, this is exactly what you need, and ideally the compiler's help to not accidentally introduce some. hash functions - no failure. etc. it's when you start walking up the abstraction ladder and into "user-level" code that errors, defects and the like start appearing and taking up (mental/code) space.

I agree that switches are bad - in addition to dialects, they also tend to not be implementable in user code, meaning that we can't build "system-like" types ie the new panics:on mode, I cannot correctly implement it in a bigint library in such a way that it acts like the system types it tries to emulate.

@arnetheduck
Copy link
Contributor Author

so.. y/n? if y, I'll go ahead and fix those silly tests

@timotheecour
Copy link
Member

timotheecour commented Apr 9, 2020

@timotheecour Switches like these mean that we effectively have different language dialects.

not really, different language dialects is when you need to re-compile nim; what I'm suggesting is just an extension of an existing compilation flag (eg --panic:AssertionError:on), and is no more a different language dialect than --panics:on/off or --threads

IMHO we should simply not track Defects in the type system.

IMO the sane behavior is assume any defect can be raised, unless specified otherwise by proc signature (and this should be rare, ie only in hotspots) or inferred by static analysis. Furthermore, mismatch in tracked defects shouldn't result in sigmatch incompatibilities.
This is for practical reasons; adding or removing an assert inside a proc shouldn't break backward compatibility; and listing all the possible defects is impractical, as you described (eg IndexError, AssertionError + ones you have even less control over eg memory etc)

Even if your proc doesn't do any arithmetic and no allocation whatsoever (not sure I've ever written such a proc btw!) the call itself could fail due to a StackOverflow.

on that note, StackOverflowError is defined but never used (probably should be raised for callDepthLimitReached but right now this calls quit)

@arnetheduck

so.. y/n? if y, I'll go ahead and fix those silly tests

one cost to consider is a bunch of deprecation messages, and incompatibilities bw nim 1.0 or 1.2 and nim devel for users that want to supress warnings by changing those types to new names.

But I'm really thinking --panic:AssertionError:on (etc) makes more sense, because as you've mentioned yourself, the distinction is often arbitrary (therefore suboptimal for performance reasons)

@Araq
Copy link
Member

Araq commented Apr 10, 2020

not really, different language dialects is when you need to re-compile nim; what I'm suggesting is just an extension of an existing compilation flag (eg --panic:AssertionError:on), and is no more a different language dialect than --panics:on/off or --threads

What a bizzare definition of language dialect, so GCC doesn't support multiple versions/dialects of C then because I don't have to recompile GCC itself in order to get them?

Indeed, --threads:on is a minor language dialect, which is why there is an RFC to turn it on always. Same for --panics:off, in fact the new exception handling caused a dialect, people complained until I made it compatible.

@arnetheduck
Copy link
Contributor Author

one cost to consider is a bunch of deprecation messages,

they're rarely raised and should never be caught - this is just to get a more reasonable error message when compiling, so that risks can be identified more quickly in software - knowing that a function can defect is critical.

arbitrary

favourite example: a[i] :)

The distinction between Error and Defect is subjective,
context-dependent and somewhat arbitrary, so when looking at an
exception, it's hard to guess what it is - this happens often when
looking at a `raises` list _without_ opening the corresponding
definition and digging through layers of inheritance.

With the help of a little consistency in naming, it's at least possible
to start disentangling the two error types and the standard lib can set
a good example here.
@arnetheduck arnetheduck force-pushed the defect-names branch 2 times, most recently from f98630d to 85ee99b Compare April 28, 2020 12:40
@Araq Araq merged commit 7d6cbf2 into nim-lang:devel Apr 28, 2020
@arnetheduck
Copy link
Contributor Author

1.2?

@alaviss
Copy link
Collaborator

alaviss commented Apr 28, 2020

New features/breaking changes don't go to 1.2.

@timotheecour
Copy link
Member

New features/breaking changes don't go to 1.2.

this can't work; see nim-lang/RFCs#214

@arnetheduck
Copy link
Contributor Author

features/breaking changes don't go to 1.2.

this is a matter of definition - this could just as well be considered a bugfix and part of the original Defect introduction - the nice thing about unreasonable backwards compat claims is that nobody is surprised when they're circumvented with this kind of arguments ;)

@Araq
Copy link
Member

Araq commented Apr 29, 2020

the nice thing about unreasonable backwards compat claims

There is nothing like that going on here. What's really going on here is that you take "semver is good" as an axiom and whenever it's demonstrated to be actually a poorly thought out concept which doesn't match a reality where bugfixes cause regressions you need to patch your definitions in a quite silly way. So now the old name "FIeldError" which we used for a decade always was a "bug", come on.

@timotheecour
Copy link
Member

timotheecour commented Apr 29, 2020

the nice thing about unreasonable backwards compat claims

the nice thing about having a CI that now runs in stable branches + important_packages is we can test things out and see what breaks in practice on a not-so-small sample size instead of FUD-ing about what could potentially break. Semver, like all good rules, should be broken when reasonable. Breakages are no fun and should be minimzed to high degree but can't be eliminated, even if you stick to bug fixes.

Other factors can matter more than 0% breakage: what packages are available for an old version; how much work is needed to support old versions, etc. If you want 0% breakage, you might as well not upgrade anything.

EchoPouet pushed a commit to EchoPouet/Nim that referenced this pull request Jun 13, 2020
* Error -> Defect for defects

The distinction between Error and Defect is subjective,
context-dependent and somewhat arbitrary, so when looking at an
exception, it's hard to guess what it is - this happens often when
looking at a `raises` list _without_ opening the corresponding
definition and digging through layers of inheritance.

With the help of a little consistency in naming, it's at least possible
to start disentangling the two error types and the standard lib can set
a good example here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants